[DAG][X86]added shrd in combineor for bzhiq+shlq+or #125734

shalini-nik · 2025-02-04T18:11:00Z

Created pull request for [DAG][X86] Use shrdq to insert into a bitfield

Fixes #112488 issue

github-actions · 2025-02-04T18:11:28Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-02-04T18:12:01Z

@llvm/pr-subscribers-backend-x86

Author: shalininikhil (shalini-nik)

Changes

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

2 Files Affected:

(modified) llvm/lib/Target/X86/X86ISelLowering.cpp (+29)
(added) llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll (+18)

diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index a956074e50d86f7..395c0f5504d1b78 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -51887,6 +51887,35 @@ static SDValue combineOr(SDNode *N, SelectionDAG &DAG,
     }
   }
 
+  if (N0.getOpcode() == ISD::SHL || N1.getOpcode() == ISD::SHL){
+    SDValue SHL = (N0.getOpcode() == ISD::SHL) ? N0 : N1;
+    SDValue OtherOp = (N0.getOpcode() == ISD::SHL) ? N1 : N0;
+    
+    if (OtherOp.getOpcode() == ISD::AND) {
+      SDValue andop = OtherOp;
+      
+      if(andop.getOperand(0).getOpcode()==ISD::Constant||andop.getOperand(1).getOpcode()==ISD::Constant){
+              
+              SDValue constOp = andop.getOperand(0).getOpcode()==ISD::Constant ? andop.getOperand(0): andop.getOperand(1);
+              SDValue valueOp = andop.getOperand(0).getOpcode()==ISD::Constant ? andop.getOperand(1): andop.getOperand(0);
+              auto *ConstRHS = dyn_cast<ConstantSDNode>(constOp);
+              uint64_t maskValue = ConstRHS->getZExtValue();
+              auto *ConstSHL = dyn_cast<ConstantSDNode>(SHL.getOperand(1));
+              uint64_t shiftValue = ConstSHL->getZExtValue();
+              
+              if((((uint64_t)1<<shiftValue)-1)==maskValue){
+                      unsigned numbits = SHL.getScalarValueSizeInBits();
+                      unsigned newshift=numbits-shiftValue;
+                      
+                      SDValue newSHL = DAG.getNode(ISD::SHL,dl,VT,valueOp,DAG.getConstant(newshift, dl, MVT::i8));
+                      SDValue R = DAG.getNode(ISD::FSHR,dl,VT,
+                                    SHL.getOperand(0),newSHL,DAG.getConstant(newshift, dl, MVT::i8));
+                      return R;
+            }
+          }
+      }
+  }
+  
   if (SDValue SetCC = combineAndOrForCcmpCtest(N, DAG, DCI, Subtarget))
     return SetCC;
 
diff --git a/llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll b/llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll
new file mode 100644
index 000000000000000..cc205ee145d88c8
--- /dev/null
+++ b/llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll
@@ -0,0 +1,18 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O3 < %s | FileCheck %s
+
+define dso_local i64 @updateTop10Bits(i64 noundef %A, i64 noundef %B) local_unnamed_addr #0 {
+; CHECK-LABEL: updateTop10Bits:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movq	%rdi, %rax
+; CHECK-NEXT:    shlq	$10, %rax
+; CHECK-NEXT:    shrdq	$10, %rsi, %rax
+; CHECK-NEXT:    retq
+entry:
+  %and = and i64 %A, 18014398509481983
+  %shl = shl i64 %B, 54
+  %or = or disjoint i64 %shl, %and
+  ret i64 %or
+}
+
+attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="skylake" "target-features"="+adx,+aes,+avx,+avx2,+bmi,+bmi2,+clflushopt,+cmov,+crc32,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+prfchw,+rdrnd,+rdseed,+sahf,+sgx,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" }
\ No newline at end of file

llvm/lib/Target/X86/X86ISelLowering.cpp

RKSimon · 2025-02-04T18:17:47Z

llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll

@@ -0,0 +1,18 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O3 < %s | FileCheck %s


No need for -O3

You should add a second RUN line with "-mattr=+slow-shld" as well to account for AMD CPUs with slow funnel shifts:

; RUN: llc < %s -mtriple=x86_64-- | FileCheck %s --check-prefixes=CHECK,FAST ; RUN: llc < %s -mtriple=x86_64-- -mattr=+slow-shld | FileCheck %s --check-prefixes=CHECK,SLOW

RKSimon · 2025-02-04T18:18:09Z

llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll

+  ret i64 %or
+}
+
+attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="skylake" "target-features"="+adx,+aes,+avx,+avx2,+bmi,+bmi2,+clflushopt,+cmov,+crc32,+cx16,+cx8,+f16c,+fma,+fsgsbase,+fxsr,+invpcid,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+prfchw,+rdrnd,+rdseed,+sahf,+sgx,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" }


remove this - attributes shouldn't be necessary

RKSimon · 2025-02-04T18:19:37Z

llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll

@@ -0,0 +1,18 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py


rename shrdq-to-insert-into-bitfield.ll to something more general "insert-bitfield.ll"?

RKSimon · 2025-02-04T18:20:01Z

llvm/test/CodeGen/X86/shrdq-to-insert-into-bitfield.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O3 < %s | FileCheck %s
+
+define dso_local i64 @updateTop10Bits(i64 noundef %A, i64 noundef %B) local_unnamed_addr #0 {


remove dso_local / noundef / local_unnamed_addr #0 noise

RKSimon · 2025-02-05T07:56:58Z

Are you working with @dipeshs809 on this?

shalini-nik · 2025-02-05T11:33:45Z

Are you working with @dipeshs809 on this?

No, i am working individually and this is my first issue

phoebewang · 2025-02-05T12:41:21Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+  if(sd_match(N,m_Or(m_Shl(m_Value(B),m_ConstInt(ShlConst)),m_And(m_Value(A),m_ConstInt(MaskConst))))){
+    uint64_t shiftValue = ShlConst.getZExtValue();
+    if(MaskConst.isMask(shiftValue)){
+      unsigned numbits = B.getScalarValueSizeInBits();
+      unsigned newshift=numbits-shiftValue;
+      SDValue newSHL = DAG.getNode(ISD::SHL,dl,VT,A,DAG.getConstant(newshift, dl, MVT::i8));
+      SDValue R = DAG.getNode(ISD::FSHR,dl,VT,B,newSHL,DAG.getConstant(newshift, dl, MVT::i8));
+      return R;
+    }
+  }


Please format the code with clang-format. I'm surprised GH didn't complain this time.

RKSimon · 2025-02-05T13:13:28Z

@shalini-nik - @dipeshs809 is OK with you taking over their assigned issue, but in the future please check an issue hasn't been assigned (or ask on the issue thread if they are still working on it).

llvm/lib/Target/X86/X86ISelLowering.cpp

…h getShiftAmountConstant

shalini-nik · 2025-02-13T07:35:22Z

@shalini-nik - @dipeshs809 is OK with you taking over their assigned issue, but in the future please check an issue hasn't been assigned (or ask on the issue thread if they are still working on it).

@RKSimon sure i will check and ask for assignment.

github-actions · 2025-02-13T08:39:34Z

✅ With the latest revision this PR passed the C/C++ code formatter.

RKSimon

please can you fix the clang-format warnings?

RKSimon

Thanks - the next step should be to add additional test coverage - different types, maybe some negative cases etc.

phoebewang · 2025-03-20T07:57:27Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+                         m_And(m_Value(A), m_ConstInt(MaskConst))))) {
+      uint64_t ShiftValue = ShlConst.getZExtValue();
+      if (MaskConst.isMask(ShiftValue) && (A.getOpcode() == ISD::CopyFromReg) &&
+          (B.getOpcode() == ISD::CopyFromReg)) {


Why do we need to check CopyFromReg?

Any idea here?

Using CopyFromReg feels like you're trying to prevent regressions on something?

phoebewang · 2025-03-20T07:59:06Z

llvm/test/CodeGen/X86/insert-bitfield.ll

+; RUN: llc < %s -mtriple=x86_64-- | FileCheck %s --check-prefixes=X64,X64-FAST
+; RUN: llc < %s -mtriple=x86_64-- -mattr=+slow-shld | FileCheck %s --check-prefixes=X64,X64-SLOW
+
+define  i64 @updateTop10Bits_64bits(i64  %A, i64  %B) {


Replace 2 spaces with one.

RKSimon · 2025-03-20T11:58:23Z

llvm/test/CodeGen/X86/insert-bitfield.ll

+; X64-SLOW-NEXT:    shlq    $54, %rsi
+; X64-SLOW-NEXT:    orq     %rsi, %rax
+; X64-SLOW-NEXT:    retq
+entry:


remove entry: lines

RKSimon · 2025-03-21T08:56:46Z

@shalini-nik have you been able to create an alive2 proof for your solution yet please?

RKSimon · 2025-03-21T08:58:12Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+        unsigned NumBits = B.getScalarValueSizeInBits();
+        unsigned NewShift = NumBits - ShiftValue;
+        if (ShiftValue > 4 && ShiftValue != 8 && ShiftValue != 16 &&
+            ShiftValue != 32 && ShiftValue != 64) {


Why are these specific shift values omitted? 64 in particular seems weird.

RKSimon · 2025-03-21T08:58:53Z

llvm/lib/Target/X86/X86ISelLowering.cpp

@@ -51887,6 +51887,47 @@ static SDValue combineOr(SDNode *N, SelectionDAG &DAG,
    }
  }

+  if (!Subtarget.isSHLDSlow()) {


There's not a single comment in this entire patch :(

RKSimon · 2025-03-21T08:59:16Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+                         m_And(m_Value(A), m_ConstInt(MaskConst))))) {
+      uint64_t ShiftValue = ShlConst.getZExtValue();
+      if (MaskConst.isMask(ShiftValue) && (A.getOpcode() == ISD::CopyFromReg) &&
+          (B.getOpcode() == ISD::CopyFromReg)) {


Using CopyFromReg feels like you're trying to prevent regressions on something?

RKSimon · 2025-04-01T12:24:12Z

@shalini-nik reverse-ping

shalini-nik · 2025-04-01T12:47:15Z

Hey @RKSimon, just saw your ping!

RKSimon

You still need to remove the unnecessary CopyFromReg checks and cleanup the ShiftVal limits (it feels like you're trying to hide regressions)

RKSimon · 2025-04-09T09:41:04Z

please can you regenerate the failing test files?

[DAG][X86]added shrd in combineor for bzhiq+shlq+or

625e962

llvmbot added the backend:X86 label Feb 4, 2025

RKSimon requested review from RKSimon and phoebewang February 4, 2025 18:14

RKSimon reviewed Feb 4, 2025

View reviewed changes

using sd_match to match this pattern

c4b8739

RKSimon mentioned this pull request Feb 5, 2025

[DAG][X86] Use shrdq to insert into a bitfield #112488

Open

phoebewang reviewed Feb 5, 2025

View reviewed changes

RKSimon reviewed Feb 5, 2025

View reviewed changes

llvm/lib/Target/X86/X86ISelLowering.cpp Show resolved Hide resolved

llvm/lib/Target/X86/X86ISelLowering.cpp Show resolved Hide resolved

formated the code with clang-format and Replaced both getConstant wit…

6d3f114

…h getShiftAmountConstant

RKSimon reviewed Feb 13, 2025

View reviewed changes

shalini-nik added 2 commits February 14, 2025 12:42

[X86] adding checks for slow-shld and fixing the clang-format warnings

d8b8f12

[X86] updating test cases

c4d62a1

RKSimon reviewed Feb 14, 2025

View reviewed changes

shalini-nik added 3 commits March 19, 2025 07:01

[X86] added checks for operands of and & shl instructions

de68b5f

[X86] added 32,16,8 bit test cases

7ffbcd6

[X86] adding missed checks

e014f9b

shalini-nik requested review from RKSimon and phoebewang March 20, 2025 02:24

phoebewang reviewed Mar 20, 2025

View reviewed changes

RKSimon reviewed Mar 20, 2025

View reviewed changes

[X86] removed 2 spaces and entry: from test cases

d759152

shalini-nik requested review from RKSimon and phoebewang March 20, 2025 18:07

RKSimon reviewed Mar 21, 2025

View reviewed changes

[X86] adding comments for the path

bc588fd

shalini-nik requested a review from RKSimon April 1, 2025 12:59

RKSimon requested changes Apr 1, 2025

View reviewed changes

[X86]removed copyfromreg check and shiftvalue check

13d8fd7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DAG][X86]added shrd in combineor for bzhiq+shlq+or #125734

[DAG][X86]added shrd in combineor for bzhiq+shlq+or #125734

shalini-nik commented Feb 4, 2025 •

edited by RKSimon

Loading

github-actions bot commented Feb 4, 2025

llvmbot commented Feb 4, 2025

RKSimon Feb 4, 2025

RKSimon Feb 4, 2025

RKSimon Feb 4, 2025

RKSimon Feb 4, 2025

RKSimon commented Feb 5, 2025

shalini-nik commented Feb 5, 2025 •

edited

Loading

phoebewang Feb 5, 2025

RKSimon commented Feb 5, 2025

shalini-nik commented Feb 13, 2025

github-actions bot commented Feb 13, 2025 •

edited

Loading

RKSimon left a comment

RKSimon left a comment

phoebewang Mar 20, 2025

phoebewang Mar 21, 2025

RKSimon Mar 21, 2025

phoebewang Mar 20, 2025

RKSimon Mar 20, 2025

RKSimon commented Mar 21, 2025

RKSimon Mar 21, 2025

RKSimon Mar 21, 2025

RKSimon Mar 21, 2025

RKSimon commented Apr 1, 2025

shalini-nik commented Apr 1, 2025

RKSimon left a comment

RKSimon commented Apr 9, 2025

		@@ -0,0 +1,18 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
		; RUN: llc -O3 < %s \| FileCheck %s

[DAG][X86]added shrd in combineor for bzhiq+shlq+or #125734

Are you sure you want to change the base?

[DAG][X86]added shrd in combineor for bzhiq+shlq+or #125734

Conversation

shalini-nik commented Feb 4, 2025 • edited by RKSimon Loading

github-actions bot commented Feb 4, 2025

llvmbot commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKSimon commented Feb 5, 2025

shalini-nik commented Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

RKSimon commented Feb 5, 2025

shalini-nik commented Feb 13, 2025

github-actions bot commented Feb 13, 2025 • edited Loading

RKSimon left a comment

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKSimon commented Mar 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKSimon commented Apr 1, 2025

shalini-nik commented Apr 1, 2025

RKSimon left a comment

Choose a reason for hiding this comment

RKSimon commented Apr 9, 2025

shalini-nik commented Feb 4, 2025 •

edited by RKSimon

Loading

shalini-nik commented Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading