[X86][GlobalIsel] fix regbank issue fabs #145674

mahesh-attarde · 2025-06-25T11:13:31Z

Fixes hidden issue in #136718.
It removes custom selection code since problem relied in regbank assignment

llvmbot · 2025-06-25T11:15:21Z

@llvm/pr-subscribers-backend-x86

Author: Mahesh-Attarde (mahesh-attarde)

Changes

Fixes hidden issue in #136718.
It removes custom selection code since problem relied in regbank assignment

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

4 Files Affected:

(modified) llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp (-33)
(modified) llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp (+2-1)
(modified) llvm/lib/Target/X86/GISel/X86RegisterBankInfo.cpp (+4-3)
(modified) llvm/test/CodeGen/X86/isel-fabs-x87.ll (+54-23)

diff --git a/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp b/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
index 9b0dd0562cde3..3090ad313b90d 100644
--- a/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
+++ b/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
@@ -95,8 +95,6 @@ class X86InstructionSelector : public InstructionSelector {
                  MachineFunction &MF) const;
   bool selectFCmp(MachineInstr &I, MachineRegisterInfo &MRI,
                   MachineFunction &MF) const;
-  bool selectFAbs(MachineInstr &I, MachineRegisterInfo &MRI,
-                  MachineFunction &MF) const;
   bool selectUAddSub(MachineInstr &I, MachineRegisterInfo &MRI,
                      MachineFunction &MF) const;
   bool selectDebugInstr(MachineInstr &I, MachineRegisterInfo &MRI) const;
@@ -393,8 +391,6 @@ bool X86InstructionSelector::select(MachineInstr &I) {
   switch (I.getOpcode()) {
   default:
     return false;
-  case TargetOpcode::G_FABS:
-    return selectFAbs(I, MRI, MF);
   case TargetOpcode::G_STORE:
   case TargetOpcode::G_LOAD:
     return selectLoadStoreOp(I, MRI, MF);
@@ -1054,35 +1050,6 @@ bool X86InstructionSelector::selectCmp(MachineInstr &I,
   I.eraseFromParent();
   return true;
 }
-bool X86InstructionSelector::selectFAbs(MachineInstr &I,
-                                        MachineRegisterInfo &MRI,
-                                        MachineFunction &MF) const {
-  assert((I.getOpcode() == TargetOpcode::G_FABS) && "unexpected instruction");
-  Register SrcReg = I.getOperand(1).getReg();
-  Register DstReg = I.getOperand(0).getReg();
-  LLT Ty = MRI.getType(SrcReg);
-  unsigned OpAbs;
-  const TargetRegisterClass *DstRC;
-  switch (Ty.getSizeInBits()) {
-  default:
-    return false;
-  case 32:
-    OpAbs = X86::ABS_Fp32;
-    DstRC = &X86::FR32RegClass;
-    break;
-  case 64:
-    OpAbs = X86::ABS_Fp64;
-    DstRC = &X86::FR64RegClass;
-    break;
-  }
-  MRI.setRegClass(DstReg, DstRC);
-  MachineInstr &FAbsInst =
-      *BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(OpAbs), DstReg)
-           .addReg(SrcReg);
-  constrainSelectedInstRegOperands(FAbsInst, TII, TRI, RBI);
-  I.eraseFromParent();
-  return true;
-}
 
 bool X86InstructionSelector::selectFCmp(MachineInstr &I,
                                         MachineRegisterInfo &MRI,
diff --git a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
index 0250ec66c0b99..34f29c555c4a4 100644
--- a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
+++ b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
@@ -419,7 +419,8 @@ X86LegalizerInfo::X86LegalizerInfo(const X86Subtarget &STI,
       .legalFor(UseX87, {s80});
 
   getActionDefinitionsBuilder(G_FABS)
-      .legalFor(UseX87 && !HasSSE2 && !HasSSE1, {s64, s80})
+      .legalFor(UseX87 && !HasSSE2 && !HasSSE1, {s80})
+      .legalFor(UseX87 && !HasSSE2 && !HasSSE1 && !Is64Bit, {s64})
       .lower();
 
   // fp comparison
diff --git a/llvm/lib/Target/X86/GISel/X86RegisterBankInfo.cpp b/llvm/lib/Target/X86/GISel/X86RegisterBankInfo.cpp
index 74fe5fcaab6cc..b23d791501729 100644
--- a/llvm/lib/Target/X86/GISel/X86RegisterBankInfo.cpp
+++ b/llvm/lib/Target/X86/GISel/X86RegisterBankInfo.cpp
@@ -341,6 +341,7 @@ X86RegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
                     /* Predicate */ PMI_None, FpRegBank, FpRegBank};
     break;
   }
+  case TargetOpcode::G_FABS:
   case TargetOpcode::G_TRUNC:
   case TargetOpcode::G_ANYEXT: {
     auto &Op0 = MI.getOperand(0);
@@ -354,9 +355,9 @@ X86RegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
         Ty0.getSizeInBits() == 128 &&
         (Ty1.getSizeInBits() == 32 || Ty1.getSizeInBits() == 64) &&
         Opc == TargetOpcode::G_ANYEXT;
-
-    getInstrPartialMappingIdxs(MI, MRI, /* isFP= */ isFPTrunc || isFPAnyExt,
-                               OpRegBankIdx);
+    bool isFAbs = (Opc == TargetOpcode::G_FABS);
+    getInstrPartialMappingIdxs(
+        MI, MRI, /* isFP= */ isFPTrunc || isFPAnyExt || isFAbs, OpRegBankIdx);
     break;
   }
   case TargetOpcode::G_LOAD: {
diff --git a/llvm/test/CodeGen/X86/isel-fabs-x87.ll b/llvm/test/CodeGen/X86/isel-fabs-x87.ll
index a0534e6a1a82e..5579754a54b46 100644
--- a/llvm/test/CodeGen/X86/isel-fabs-x87.ll
+++ b/llvm/test/CodeGen/X86/isel-fabs-x87.ll
@@ -1,29 +1,44 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
-; RUN: llc < %s -mtriple=x86_64-- -mattr=+x87,-sse2,-sse | FileCheck %s --check-prefixes=X64
-; RUN: llc < %s -mtriple=x86_64-- -mattr=+x87,-sse2,-sse -fast-isel | FileCheck %s --check-prefixes=X64
-; RUN: llc < %s -mtriple=x86_64-- -mattr=+x87,-sse2,-sse -global-isel -global-isel-abort=1 | FileCheck %s --check-prefixes=X64
-; RUN: llc < %s -mtriple=i686-- -mattr=+x87,-sse2,-sse | FileCheck %s --check-prefixes=X86,SDAG-ISEL
-; RUN: llc < %s -mtriple=i686-- -mattr=+x87,-sse2,-sse -fast-isel | FileCheck %s --check-prefixes=X86,Fast-ISEL
-; RUN: llc < %s -mtriple=i686-- -mattr=+x87,-sse2,-sse -global-isel -global-isel-abort=0 | FileCheck %s --check-prefixes=X86,GISEL-ISEL
+; RUN: llc < %s -mtriple=x86_64-- -mattr=+x87,-sse2,-sse | FileCheck %s --check-prefixes=X64,SDAG-X64-ISEL
+; RUN: llc < %s -mtriple=x86_64-- -mattr=+x87,-sse2,-sse -fast-isel | FileCheck %s --check-prefixes=X64,FAST-X64-ISEL
+; RUN: llc < %s -mtriple=x86_64-- -mattr=+x87,-sse2,-sse -global-isel -global-isel-abort=1 | FileCheck %s --check-prefixes=X64,GISEL-X64-ISEL
+; RUN: llc < %s -mtriple=i686-- -mattr=+x87,-sse2,-sse | FileCheck %s --check-prefixes=X86,SDAG-X86-ISEL
+; RUN: llc < %s -mtriple=i686-- -mattr=+x87,-sse2,-sse -fast-isel | FileCheck %s --check-prefixes=X86,FAST-X86-ISEL
+; RUN: llc < %s -mtriple=i686-- -mattr=+x87,-sse2,-sse -global-isel -global-isel-abort=0 | FileCheck %s --check-prefixes=X86,GISEL-X86-ISEL
 
 define void @test_float_abs(ptr %argptr)   {
-; SDAG-ISEL-LABEL: test_float_abs:
-; SDAG-ISEL:       # %bb.0:
-; SDAG-ISEL-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; SDAG-ISEL-NEXT:    andb $127, 3(%eax)
-; SDAG-ISEL-NEXT:    retl
-;
-; Fast-ISEL-LABEL: test_float_abs:
-; Fast-ISEL:       # %bb.0:
-; Fast-ISEL-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; Fast-ISEL-NEXT:    andb $127, 3(%eax)
-; Fast-ISEL-NEXT:    retl
-;
-; GISEL-ISEL-LABEL: test_float_abs:
-; GISEL-ISEL:       # %bb.0:
-; GISEL-ISEL-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; GISEL-ISEL-NEXT:    andl $2147483647, (%eax) # imm = 0x7FFFFFFF
-; GISEL-ISEL-NEXT:    retl
+; SDAG-X64-ISEL-LABEL: test_float_abs:
+; SDAG-X64-ISEL:       # %bb.0:
+; SDAG-X64-ISEL-NEXT:    andb $127, 3(%rdi)
+; SDAG-X64-ISEL-NEXT:    retq
+;
+; FAST-X64-ISEL-LABEL: test_float_abs:
+; FAST-X64-ISEL:       # %bb.0:
+; FAST-X64-ISEL-NEXT:    andb $127, 3(%rdi)
+; FAST-X64-ISEL-NEXT:    retq
+;
+; GISEL-X64-ISEL-LABEL: test_float_abs:
+; GISEL-X64-ISEL:       # %bb.0:
+; GISEL-X64-ISEL-NEXT:    andl $2147483647, (%rdi) # imm = 0x7FFFFFFF
+; GISEL-X64-ISEL-NEXT:    retq
+;
+; SDAG-X86-ISEL-LABEL: test_float_abs:
+; SDAG-X86-ISEL:       # %bb.0:
+; SDAG-X86-ISEL-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; SDAG-X86-ISEL-NEXT:    andb $127, 3(%eax)
+; SDAG-X86-ISEL-NEXT:    retl
+;
+; FAST-X86-ISEL-LABEL: test_float_abs:
+; FAST-X86-ISEL:       # %bb.0:
+; FAST-X86-ISEL-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; FAST-X86-ISEL-NEXT:    andb $127, 3(%eax)
+; FAST-X86-ISEL-NEXT:    retl
+;
+; GISEL-X86-ISEL-LABEL: test_float_abs:
+; GISEL-X86-ISEL:       # %bb.0:
+; GISEL-X86-ISEL-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; GISEL-X86-ISEL-NEXT:    andl $2147483647, (%eax) # imm = 0x7FFFFFFF
+; GISEL-X86-ISEL-NEXT:    retl
      %arg = load  float, float* %argptr
      %abs = tail call float @llvm.fabs.f32(float %arg)
      store float %abs, ptr %argptr
@@ -31,6 +46,22 @@ define void @test_float_abs(ptr %argptr)   {
  }
 
 define void @test_double_abs(ptr %argptr)  {
+; SDAG-X64-ISEL-LABEL: test_double_abs:
+; SDAG-X64-ISEL:       # %bb.0:
+; SDAG-X64-ISEL-NEXT:    andb $127, 7(%rdi)
+; SDAG-X64-ISEL-NEXT:    retq
+;
+; FAST-X64-ISEL-LABEL: test_double_abs:
+; FAST-X64-ISEL:       # %bb.0:
+; FAST-X64-ISEL-NEXT:    andb $127, 7(%rdi)
+; FAST-X64-ISEL-NEXT:    retq
+;
+; GISEL-X64-ISEL-LABEL: test_double_abs:
+; GISEL-X64-ISEL:       # %bb.0:
+; GISEL-X64-ISEL-NEXT:    movabsq $9223372036854775807, %rax # imm = 0x7FFFFFFFFFFFFFFF
+; GISEL-X64-ISEL-NEXT:    andq %rax, (%rdi)
+; GISEL-X64-ISEL-NEXT:    retq
+;
 ; X86-LABEL: test_double_abs:
 ; X86:       # %bb.0:
 ; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax

e-kud · 2025-06-26T08:15:31Z

llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp

@@ -419,7 +419,8 @@ X86LegalizerInfo::X86LegalizerInfo(const X86Subtarget &STI,
      .legalFor(UseX87, {s80});

  getActionDefinitionsBuilder(G_FABS)
-      .legalFor(UseX87 && !HasSSE2 && !HasSSE1, {s64, s80})
+      .legalFor(UseX87 && !HasSSE2 && !HasSSE1, {s80})


I think there is no need for SSE checks.

Agreed - x87_f80 is only available with UseX87 - SSE availability is irrelevant

e-kud · 2025-06-26T08:17:59Z

llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp

@@ -419,7 +419,8 @@ X86LegalizerInfo::X86LegalizerInfo(const X86Subtarget &STI,
      .legalFor(UseX87, {s80});

  getActionDefinitionsBuilder(G_FABS)
-      .legalFor(UseX87 && !HasSSE2 && !HasSSE1, {s64, s80})
+      .legalFor(UseX87 && !HasSSE2 && !HasSSE1, {s80})
+      .legalFor(UseX87 && !HasSSE2 && !HasSSE1 && !Is64Bit, {s64})


Do we really need !HasSSE1 here? Does lower work with SSE1?

e-kud

LGTM. Thanks!

mahesh-attarde · 2025-06-30T13:04:03Z

ping @RKSimon

RKSimon

LGTM

[X86][GlobalIsel] fix regbank issue fabs

1bf7cec

mahesh-attarde marked this pull request as ready for review June 25, 2025 11:14

llvmbot added the backend:X86 label Jun 25, 2025

RKSimon requested a review from e-kud June 25, 2025 12:03

e-kud reviewed Jun 26, 2025

View reviewed changes

fix 87 only legalize

74c2324

e-kud approved these changes Jun 29, 2025

View reviewed changes

Merge branch 'main' into fix_regbank_fabs

6f2a059

RKSimon approved these changes Jun 30, 2025

View reviewed changes

Merge branch 'main' into fix_regbank_fabs

b86dab6

e-kud merged commit 56ef00a into llvm:main Jun 30, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[X86][GlobalIsel] fix regbank issue fabs #145674

[X86][GlobalIsel] fix regbank issue fabs #145674

mahesh-attarde commented Jun 25, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jun 25, 2025

Uh oh!

e-kud Jun 26, 2025

Uh oh!

RKSimon Jun 26, 2025

Uh oh!

mahesh-attarde Jun 27, 2025

Uh oh!

e-kud Jun 26, 2025

Uh oh!

e-kud left a comment

Uh oh!

mahesh-attarde commented Jun 30, 2025

Uh oh!

RKSimon left a comment

Uh oh!

Uh oh!

Uh oh!

[X86][GlobalIsel] fix regbank issue fabs #145674

[X86][GlobalIsel] fix regbank issue fabs #145674

Conversation

mahesh-attarde commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 25, 2025

Uh oh!

e-kud Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

RKSimon Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

mahesh-attarde Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

e-kud Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

e-kud left a comment

Choose a reason for hiding this comment

Uh oh!

mahesh-attarde commented Jun 30, 2025

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mahesh-attarde commented Jun 25, 2025 •

edited

Loading