Skip to content

[X86][GlobalIsel] G_BITCAST support #144473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mahesh-attarde
Copy link
Contributor

Adds support for G_BITCAST

; RUN: llc < %s -mtriple=i686--
; RUN: llc < %s -mtriple=x86_64--
; RUN: llc < %s -mtriple=x86_64-- -global-isel -global-isel-abort=1 | FileCheck %s -check-prefixes=GISEL
; XRUN: llc < %s -mtriple=i686-- -global-isel -global-isel-abort=1 | FileCheck %s -check-prefixes=GISEL
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i686 run is WIP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

LegalizerHelper &Helper) const {
MachineIRBuilder &MIRBuilder = Helper.MIRBuilder;
auto [DstReg, DstTy, SrcReg, SrcTy] = MI.getFirst2RegLLTs();
assert(!SrcTy.isVector() && "G_BITCAST does not support vectors yet");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct to put assert there? It may be better to return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. It seems like multiple recurring pattern though

assert((SrcTy.getSizeInBits() == 16 || SrcTy.getSizeInBits() == 32 ||

assert((Opc == TargetOpcode::G_STORE || Opc == TargetOpcode::G_LOAD) &&

@llvmbot
Copy link
Member

llvmbot commented Jun 19, 2025

@llvm/pr-subscribers-backend-x86

Author: Mahesh-Attarde (mahesh-attarde)

Changes

Adds support for G_BITCAST


Full diff: https://github.com/llvm/llvm-project/pull/144473.diff

3 Files Affected:

  • (modified) llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp (+19)
  • (modified) llvm/lib/Target/X86/GISel/X86LegalizerInfo.h (+2)
  • (modified) llvm/test/CodeGen/X86/bitcast.ll (+36-3)
diff --git a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
index 11dd05c584983..85e6c190c7ced 100644
--- a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
+++ b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
@@ -669,6 +669,8 @@ bool X86LegalizerInfo::legalizeCustom(LegalizerHelper &Helper, MachineInstr &MI,
     return legalizeSITOFP(MI, MRI, Helper);
   case TargetOpcode::G_FPTOSI:
     return legalizeFPTOSI(MI, MRI, Helper);
+  case TargetOpcode::G_BITCAST:
+    return legalizeBitcast(MI, MRI, Helper);
   }
   llvm_unreachable("expected switch to return");
 }
@@ -835,6 +837,23 @@ bool X86LegalizerInfo::legalizeNarrowingStore(MachineInstr &MI,
   return true;
 }
 
+bool X86LegalizerInfo::legalizeBitcast(MachineInstr &MI,
+                                       MachineRegisterInfo &MRI,
+                                       LegalizerHelper &Helper) const {
+  MachineIRBuilder &MIRBuilder = Helper.MIRBuilder;
+  auto [DstReg, DstTy, SrcReg, SrcTy] = MI.getFirst2RegLLTs();
+  assert(!SrcTy.isVector() && "G_BITCAST does not support vectors yet");
+  bool isCopy =
+      (SrcTy == DstTy) || (SrcTy.getSizeInBits() == DstTy.getSizeInBits());
+  if (isCopy) {
+    MIRBuilder.buildCopy(DstReg, SrcReg);
+    MI.eraseFromParent();
+    return true;
+  }
+  // For Vectors specific bitcasts
+  return Helper.lowerBitcast(MI) == LegalizerHelper::LegalizeResult::Legalized;
+}
+
 bool X86LegalizerInfo::legalizeIntrinsic(LegalizerHelper &Helper,
                                          MachineInstr &MI) const {
   return true;
diff --git a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.h b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.h
index 1ba82674ed4c6..eb42126d079fb 100644
--- a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.h
+++ b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.h
@@ -54,6 +54,8 @@ class X86LegalizerInfo : public LegalizerInfo {
 
   bool legalizeFPTOSI(MachineInstr &MI, MachineRegisterInfo &MRI,
                       LegalizerHelper &Helper) const;
+  bool legalizeBitcast(MachineInstr &MI, MachineRegisterInfo &MRI,
+                       LegalizerHelper &Helper) const;
 };
 } // namespace llvm
 #endif
diff --git a/llvm/test/CodeGen/X86/bitcast.ll b/llvm/test/CodeGen/X86/bitcast.ll
index 0866a0b1b2bd1..c9193fed0ed5c 100644
--- a/llvm/test/CodeGen/X86/bitcast.ll
+++ b/llvm/test/CodeGen/X86/bitcast.ll
@@ -1,24 +1,57 @@
-; RUN: llc < %s -mtriple=i686--
-; RUN: llc < %s -mtriple=x86_64--
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=i686-- | FileCheck %s -check-prefixes=X86
+; RUN: llc < %s -mtriple=x86_64--| FileCheck %s -check-prefixes=X64
+; RUN: llc < %s -mtriple=i686--   -global-isel -global-isel-abort=0 | FileCheck %s -check-prefixes=X86
+; RUN: llc < %s -mtriple=x86_64--  -global-isel -global-isel-abort=1 | FileCheck %s -check-prefixes=X64
 ; PR1033
 
 define i64 @test1(double %t) {
+; X64-LABEL: test1:
+; X64:       # %bb.0:
+; X64-NEXT:    movq %xmm0, %rax
+; X64-NEXT:    retq
         %u = bitcast double %t to i64           ; <i64> [#uses=1]
         ret i64 %u
 }
 
 define double @test2(i64 %t) {
+; X86-LABEL: test2:
+; X86:       # %bb.0:
+; X86-NEXT:    fldl {{[0-9]+}}(%esp)
+; X86-NEXT:    retl
+;
+; X64-LABEL: test2:
+; X64:       # %bb.0:
+; X64-NEXT:    movq %rdi, %xmm0
+; X64-NEXT:    retq
         %u = bitcast i64 %t to double           ; <double> [#uses=1]
         ret double %u
 }
 
 define i32 @test3(float %t) {
+; X86-LABEL: test3:
+; X86:       # %bb.0:
+; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT:    retl
+;
+; X64-LABEL: test3:
+; X64:       # %bb.0:
+; X64-NEXT:    movd %xmm0, %eax
+; X64-NEXT:    retq
         %u = bitcast float %t to i32            ; <i32> [#uses=1]
         ret i32 %u
 }
 
 define float @test4(i32 %t) {
+; X86-LABEL: test4:
+; X86:       # %bb.0:
+; X86-NEXT:    flds {{[0-9]+}}(%esp)
+; X86-NEXT:    retl
+;
+; X64-LABEL: test4:
+; X64:       # %bb.0:
+; X64-NEXT:    movd %edi, %xmm0
+; X64-NEXT:    retq
         %u = bitcast i32 %t to float            ; <float> [#uses=1]
         ret float %u
 }
-

@mahesh-attarde
Copy link
Contributor Author

Can you review ? @RKSimon @e-kud

@RKSimon RKSimon requested review from RKSimon and e-kud June 19, 2025 11:43
; RUN: llc < %s -mtriple=i686-- | FileCheck %s -check-prefixes=X86
; RUN: llc < %s -mtriple=x86_64--| FileCheck %s -check-prefixes=X64
; RUN: llc < %s -mtriple=i686-- -global-isel -global-isel-abort=0 | FileCheck %s -check-prefixes=X86
; RUN: llc < %s -mtriple=x86_64-- -global-isel -global-isel-abort=1 | FileCheck %s -check-prefixes=X64
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to create a new isel-bitcast.ll test file instead with full dag/fastisel/gisel coverage (see other isel-* files for examples)

Copy link
Contributor Author

@mahesh-attarde mahesh-attarde Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#144928 Added Test

@e-kud
Copy link
Contributor

e-kud commented Jun 19, 2025

@mahesh-attarde could you please elaborate why this change is needed because the tests pass without this PR:
https://godbolt.org/z/EEzP4nn4n

There is a problem with double conversions in test2 on i686 but it is all over there.

@mahesh-attarde
Copy link
Contributor Author

@mahesh-attarde could you please elaborate why this change is needed because the tests pass without this PR: https://godbolt.org/z/EEzP4nn4n

There is a problem with double conversions in test2 on i686 but it is all over there.

While doing FP class test we need to convert F32 to I32 and F64 to I64 and create sequence of bitwise ops.

@mahesh-attarde
Copy link
Contributor Author

@mahesh-attarde could you please elaborate why this change is needed because the tests pass without this PR: https://godbolt.org/z/EEzP4nn4n
There is a problem with double conversions in test2 on i686 but it is all over there.

While doing FP class test we need to convert F32 to I32 and F64 to I64 and create sequence of bitwise ops.

https://github.com/mahesh-attarde/llvm-project/pull/4/files#diff-67fe52af4babc3c78a4ad94f25bc84a724cd6a4aacefd6db24887d5d29c3b936R797

@e-kud
Copy link
Contributor

e-kud commented Jun 20, 2025

@mahesh-attarde could you please elaborate why this change is needed because the tests pass without this PR: https://godbolt.org/z/EEzP4nn4n
There is a problem with double conversions in test2 on i686 but it is all over there.

While doing FP class test we need to convert F32 to I32 and F64 to I64 and create sequence of bitwise ops.

Is there a test in IR or GIR that shows the problem? Because current tests only show that this PR workarounds an issue with doubles on i686.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants