[X86][SelectionDAG] Fix the Gather's base and index by modifying the Scale value #137813

rohitaggarwal007 · 2025-04-29T13:58:54Z

Fix the Gather's base and index for one use or multiple uses of Index Node. Using the approach to update the Scale if SHL Opcode and followed by truncate.

PR closed : #134979

… Node. Using the approach to update the Scale if SHL Opcode and followed by truncate.

…7/llvm-project into gatherMultipleOccurrence

…007/llvm-project into gatherMultipleOccurrence

… identify Signed bits.

…7/llvm-project into gatherMultipleOccurrence

rohitaggarwal007 · 2025-04-29T14:00:52Z

@RKSimon, This is the new PR as previous one got closed due to merge conflict(#134979 )

RKSimon

thanks - almost there!

RKSimon · 2025-05-02T16:36:00Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+          DCI.AddToWorklist(N);
+        return SDValue(N, 0);
+      }
+      uint64_t ScaleAmt = Scale->getAsZExtVal();


move this up and reuse in Log2_32 check - we should probably assert that ScaleAmt is a power of 2 as well

RKSimon · 2025-05-02T16:36:23Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+      }
+      uint64_t ScaleAmt = Scale->getAsZExtVal();
+      if (auto MinShAmt = DAG.getValidMinimumShiftAmount(Index)) {
+        if (*MinShAmt >= 1 && (*MinShAmt + Log2_64(ScaleAmt)) < 4 &&


merge Log2_64 call with the existing Log2_32 call

llvm/lib/Target/X86/X86ISelLowering.cpp

RKSimon · 2025-05-02T16:41:50Z

llvm/lib/Target/X86/X86ISelLowering.cpp

    unsigned IndexWidth = Index.getScalarValueSizeInBits();

    // Shrink indices if they are larger than 32-bits.
    // Only do this before legalize types since v2i64 could become v2i32.
    // FIXME: We could check that the type is legal if we're after legalize
    // types, but then we would need to construct test cases where that happens.
-    if (IndexWidth > 32 && DAG.ComputeNumSignBits(Index) > (IndexWidth - 32)) {
+    unsigned ComputeNumSignBits = DAG.ComputeNumSignBits(Index);
+    if (Index.getOpcode() == ISD::SHL) {


All this code needs moving inside the IndexWidth > 32 check - needs commenting as well

github-actions · 2025-05-05T06:33:33Z

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

RKSimon

LGTM with one minor

RKSimon · 2025-05-06T12:02:02Z

llvm/lib/Target/X86/X86ISelLowering.cpp

    // Shrink indices if they are larger than 32-bits.
    // Only do this before legalize types since v2i64 could become v2i32.
    // FIXME: We could check that the type is legal if we're after legalize
    // types, but then we would need to construct test cases where that happens.
-    if (IndexWidth > 32 && DAG.ComputeNumSignBits(Index) > (IndexWidth - 32)) {
+    // \ComputeNumSignBits value is recomputed for the shift Index
+    if (IndexWidth > 32 && ComputeNumSignBits > (IndexWidth - 32)) {


Probably better if you can move everything inside if (IndexWidth > 32)

@RKSimon, Done. Moved the code inside the if (IndexWidth > 32)

rohitaggarwal007 · 2025-05-07T04:33:05Z

@RKSimon Can you please merge my changes?
I don't have the write permission.

Thanks

RKSimon · 2025-05-12T09:51:16Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+    // Attempt to move shifted index into the address scale, allows further
+    // index truncation below.
+    if (Index.getOpcode() == ISD::SHL && isa<ConstantSDNode>(Scale)) {
+      unsigned BitWidth = Index.getScalarValueSizeInBits();


Hoist the equivalent IndexWidth above this and remove BitWidth

RKSimon · 2025-05-12T10:14:54Z

llvm/lib/Target/X86/X86ISelLowering.cpp

+            ComputeNumSignBits += *MinShAmt;
+          }
+        }
+      }


I don't think we need this anymore

@RKSimon, I do not understand the comment fully... Does it mean to move it outside IndexWith > 32 or Does it mean to remove this code snippet?
if (Index.getOpcode() == ISD::SHL) { if (auto MinShAmt = DAG.getValidMinimumShiftAmount(Index)) { if (DAG.ComputeNumSignBits(Index.getOperand(0)) > 1) { ComputeNumSignBits += *MinShAmt; } } }

We can not remove this code as it is required and our test case is failing.

In which case, please can you pull it out of this patch - I'm happy with the rest of it, but I'm concerned that this will only work under very specific conditions - better to pull it out and you can work on it in a follow up PR.

Incase of SHL have the value of shift 4 or greater... We are recalculating the number of signed bit so that we can truncate.

Sure, i will create a separate PR for this

Done, removed the code and created a separate PR.

RKSimon

LGTM - cheers

rohitaggarwal007 · 2025-05-13T12:19:49Z

@RKSimon, Can you please merge this on my behalf?

Thanks

…39703) Fix the Gather's Index for SHL Opcode in which shift amount is 4 or greater. It is in the continuity of #137813 --------- Co-authored-by: Rohit Aggarwal <[email protected]> Co-authored-by: Simon Pilgrim <[email protected]>

…vm#139703) Fix the Gather's Index for SHL Opcode in which shift amount is 4 or greater. It is in the continuity of llvm#137813 --------- Co-authored-by: Rohit Aggarwal <[email protected]> Co-authored-by: Simon Pilgrim <[email protected]>

Rohit Aggarwal and others added 27 commits April 9, 2025 14:27

Fix the Gather's base and index for one use or multiple uses of Index…

741acb0

… Node. Using the approach to update the Scale if SHL Opcode and followed by truncate.

Fix the Gather's base and index for one use or multiple uses of Index…

9d395b1

… Node. Using the approach to update the Scale if SHL Opcode and followed by truncate.

Merge branch 'gatherMultipleOccurrence' of github.com:rohitaggarwal00…

0625ae4

…7/llvm-project into gatherMultipleOccurrence

Merge branch 'gatherMultipleOccurrence' of github.com:rohitaggarwal00…

84d9a5f

…7/llvm-project into gatherMultipleOccurrence

Merge branch 'gatherMultipleOccurrence' of github.com:rohitaggarwal00…

76d9167

…7/llvm-project into gatherMultipleOccurrence

Merge branch 'gatherMultipleOccurrence' of github.com:rohitaggarwal00…

b73051c

…7/llvm-project into gatherMultipleOccurrence

Merge branch 'gatherMultipleOccurrence' of github.com:rohitaggarwal00…

a43aa1d

…7/llvm-project into gatherMultipleOccurrence

Changes

03b3daf

Occurrence

0e8856f

Update gatherBaseIndexFix.ll

7eb7663

squash! Changes

4ed4a4d

s Merge branch 'gatherMultipleOccurrence' of github.com:rohitaggarwal…

0c4eb0f

…007/llvm-project into gatherMultipleOccurrence

Merge branch 'llvm:main' into gatherMultipleOccurrence

dc1b6d6

Merge branch 'main' into gatherMultipleOccurrence

34df281

Merge branch 'llvm:main' into gatherMultipleOccurrence

5aaa2ab

Merge branch 'llvm:main' into gatherMultipleOccurrence

8ce9360

Update the masked_gather_scatter.ll

bfbbe9f

Remove redundant gatherBaseIndexFix.ll

4bb9f5b

Remove updateBaseIndex function and update the Checks for testcase

fb131f8

Revert back the hasOne check

47bf70b

Fold opertation

716943e

Restrict Scale so that it can happen fully or none. Added a logic for…

b9e571a

… identify Signed bits.

Add test case and merge usecases fo SHL

f0a175a

Update undef to poison in testcase

4b57e4a

Merge branch 'gatherMultipleOccurrence' of github.com:rohitaggarwal00…

8995df9

…7/llvm-project into gatherMultipleOccurrence

Update the testcase

5a12c7d

Remove the unwanted tests

d2f1352

RKSimon self-requested a review April 30, 2025 21:26

RKSimon reviewed May 2, 2025

View reviewed changes

Log2 handling and updating the comments

ed72aa3

Rohit Aggarwal and others added 2 commits May 5, 2025 12:59

Fix formatting

1644def

Merge branch 'llvm:main' into gatherMultipleOccurrence

7ca7d48

RKSimon approved these changes May 6, 2025

View reviewed changes

Code movement to if(IndexWidth > 32) condition

94f16e6

RKSimon reviewed May 12, 2025

View reviewed changes

rohitaggarwal007 and others added 3 commits May 12, 2025 16:09

Merge branch 'llvm:main' into gatherMultipleOccurrence

cfcf542

Hoist the variable to reuse it

7ab9924

Split the PR into two PRs.

62fa5ad

rohitaggarwal007 mentioned this pull request May 13, 2025

[X86][SelectionDAG] Handle the case for gather where index is SHL #139703

Merged

RKSimon approved these changes May 13, 2025

View reviewed changes

Merge branch 'main' into gatherMultipleOccurrence

ad2e419

RKSimon merged commit 75d36dc into llvm:main May 13, 2025
9 of 11 checks passed

rohitaggarwal007 deleted the gatherMultipleOccurrence branch May 13, 2025 17:49

rohitaggarwal007 mentioned this pull request May 20, 2025

Update the base and index value for masked gather #130920

Closed

[X86][SelectionDAG] Fix the Gather's base and index by modifying the Scale value #137813

[X86][SelectionDAG] Fix the Gather's base and index by modifying the Scale value #137813

Uh oh!

Conversation

rohitaggarwal007 commented Apr 29, 2025

Uh oh!

rohitaggarwal007 commented Apr 29, 2025

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohitaggarwal007 commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

rohitaggarwal007 commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 5, 2025 •

edited

Loading