[AMDGPU] Handle MachineOperandType global address in SIFoldOperands. #135424

isakhilesh · 2025-04-11T19:17:02Z

This patch handles the global operand type properly, fixing the
bug : Assertion (isFI() || isCPI() || isTargetIndex() || isJTI()) && "Wrong MachineOperand accessor" failed.

Fixes SWDEV-504645

github-actions · 2025-04-11T19:17:20Z

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-04-11T19:18:01Z

@llvm/pr-subscribers-backend-amdgpu

Author: Akhilesh Moorthy (isakhilesh)

Changes

While FoldingImmLike accepts :
MO_Immediate, MO_FrameIndex and MO_GlobalAddress.
The conditional block after it only covers the first 2. So I have added a case for the global value as well.

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

1 Files Affected:

(modified) llvm/lib/Target/AMDGPU/SIFoldOperands.cpp (+3-1)

diff --git a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
index d6acf9e081b9f..701d17930d7df 100644
--- a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
@@ -1161,8 +1161,10 @@ void SIFoldOperandsImpl::foldOperand(
 
         if (OpToFold.isImm())
           UseMI->getOperand(1).ChangeToImmediate(OpToFold.getImm());
-        else
+        else if(OpToFold.isFI())
           UseMI->getOperand(1).ChangeToFrameIndex(OpToFold.getIndex());
+        else if(OpToFold.isGlobal())
+          return;
         UseMI->removeOperand(2); // Remove exec read (or src1 for readlane)
         return;
       }

isakhilesh · 2025-04-11T19:18:18Z

Work In Progress: working on adding a test.

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

arsenm

Please do not post or update new reviews without a test.

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

Fixes SWDEV-504645. This patch handles the global operand type properly fixing the bug : Assertion `(isFI() || isCPI() || isTargetIndex() || isJTI()) && "Wrong MachineOperand accessor"' failed.

llvm/test/CodeGen/AMDGPU/swdev-504645.ll

arsenm · 2025-04-24T16:59:07Z

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

+        else if (OpToFold.isGlobal())
+          UseMI->getOperand(1).ChangeToGA(OpToFold.getGlobal(),
+                                          OpToFold.getOffset(),
+                                          OpToFold.getTargetFlags());
        else
          UseMI->getOperand(1).ChangeToFrameIndex(OpToFold.getIndex());


Maybe swap these? Handle isFI in the else if. We could also include explicit check for isGlobal, and give up on other operand types

Maybe swap these? Handle isFI in the else if. We could also include explicit check for isGlobal, and give up on other operand types

Swapping isImm() and isGlobal() seems fine, if that's what you meant.

There is a UseMI->setDesc() before this conditional block, so it was not clear whether it was too late to give up because a change may have been made already?

It seemed minimum changes compared to the existing code if the existing "else" was kept untouched and a new ifGlobal() handling was added since that was the asserting case to start with.

Yes, the setDesc would need to be sunk if the other case were handled

Hi @arsenm, I had initially considered handling isFI() in else if, but as @dhruvachak mentioned, UseMI->setDesc() has potentially made changes already, so I am keeping it in the else block since there might be modifications required with respect to setDesc() as well in case we swap them.

First, I meant do if (isImm) {} else if (isFI()) {} { else global

But you could also just move the setDesc to avoid the side effect

I can modify the patch to the first approach you mention here. Since we enter this block only in case FoldingImmLike is true and this variable is a bool for either immediate, frame index or global values. The if-else if-else conditions focus on only these 3 operands so there won't be any edge case for the else condition as we enter the block when it is one of the 3 operands.

But I'm not clear on how/where should I move the setDesc() to, since it is almost at the top of the block and the if-else if-else is immediately after it.

The structure is ->

if(FoldingImmLike){
if(execMaybeModifiedBeforeUse)
return;
UseMI->setDesc() // at the top of the block, in case the exec flag immediately above is false
if-else if-else block
UseMI->removeOperand(2)
return;
}

Where should I place setDesc() to avoid a side effect?

I can modify the patch to the first approach you mention here. Since we enter this block only in case FoldingImmLike is true and this variable is a bool for either immediate, frame index or global values. The if-else if-else conditions focus on only these 3 operands so there won't be any edge case for the else condition as we enter the block when it is one of the 3 operands.

But I'm not clear on how/where should I move the setDesc() to, since it is almost at the top of the block and the if-else if-else is immediately after it.

The structure is ->

if(FoldingImmLike){ if(execMaybeModifiedBeforeUse) return; UseMI->setDesc() // at the top of the block, in case the exec flag immediately above is false if-else if-else block UseMI->removeOperand(2) return; }

Where should I place setDesc() to avoid a side effect?

I think setDesc() has to be sunk only if you bail out. How about the following?

if (isImm()) {}
else if (isFI()) {}
else {
assert(isGlobal());
...
}
Then you don't need to move setDesc().

@dhruvachak is spelling out the same as I stated

Got it, I have updated the patch based on the suggested structure, thanks a lot. Please let me know if I can improve it further.

dhruvachak · 2025-04-24T17:36:35Z

Occurred to me that perhaps the test name should be changed since the swdev# won't make sense to everyone. What do you think @arsenm ?

arsenm · 2025-04-24T17:48:03Z

Occurred to me that perhaps the test name should be changed since the swdev# won't make sense to everyone. What do you think @arsenm ?

I usually include that as a suffix, or in the test function name with some other description

isakhilesh · 2025-04-25T04:06:07Z

Occurred to me that perhaps the test name should be changed since the swdev# won't make sense to everyone. What do you think @arsenm ?

I usually include that as a suffix, or in the test function name with some other description

Yes, updated the name of the test file.

arsenm · 2025-04-25T19:47:48Z

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

+        else if (OpToFold.isGlobal())
+          UseMI->getOperand(1).ChangeToGA(OpToFold.getGlobal(),
+                                          OpToFold.getOffset(),
+                                          OpToFold.getTargetFlags());
        else
          UseMI->getOperand(1).ChangeToFrameIndex(OpToFold.getIndex());


First, I meant do if (isImm) {} else if (isFI()) {} { else global

But you could also just move the setDesc to avoid the side effect

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

llvm/test/CodeGen/AMDGPU/swdev504645-global-fold.ll

shiltian · 2025-04-26T23:48:43Z

llvm/test/CodeGen/AMDGPU/swdev504645-global-fold.ll

+
+attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+!0 = !{!"DescriptorBuffer", i32 4, i32 8, i32 0, i32 0}


do we really need these attribute?

I have removed the attribute code in line 44 but have kept the code in line 46 because without it there was an error asking for !0.
This file was reduced in llvm-reduce from an input .ll file generated by a fuzzer based on the ticket mentioned in the description.

dhruvachak

LGTM. Please wait for approval from @arsenm .

llvm/test/CodeGen/AMDGPU/swdev504645-global-fold.ll

github-actions · 2025-05-05T16:12:54Z

@isakhilesh Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…lvm#135424) This patch handles the global operand type properly, fixing the bug : Assertion `(isFI() || isCPI() || isTargetIndex() || isJTI()) && "Wrong MachineOperand accessor"` failed. Fixes SWDEV-504645 --------- Co-authored-by: Matt Arsenault <[email protected]>

llvmbot added the backend:AMDGPU label Apr 11, 2025

arsenm reviewed Apr 11, 2025

View reviewed changes

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp Outdated Show resolved Hide resolved

dhruvachak requested review from nhaehnle, dhruvachak and ronlieb April 11, 2025 19:21

isakhilesh force-pushed the updated_global_address branch 2 times, most recently from 686a2b5 to 69c3465 Compare April 11, 2025 19:37

arsenm requested changes Apr 11, 2025

View reviewed changes

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp Outdated Show resolved Hide resolved

[AMDGPU] Handle MachineOperandType global address in SIFoldOperands.

ea3c20a

Fixes SWDEV-504645. This patch handles the global operand type properly fixing the bug : Assertion `(isFI() || isCPI() || isTargetIndex() || isJTI()) && "Wrong MachineOperand accessor"' failed.

isakhilesh force-pushed the updated_global_address branch from 69c3465 to ea3c20a Compare April 24, 2025 16:10

arsenm reviewed Apr 24, 2025

View reviewed changes

Patch with updated test case, based on feedback.

d259fb1

arsenm reviewed Apr 25, 2025

View reviewed changes

shiltian reviewed Apr 26, 2025

View reviewed changes

llvm/test/CodeGen/AMDGPU/swdev504645-global-fold.ll Outdated Show resolved Hide resolved

shiltian reviewed Apr 26, 2025

View reviewed changes

Updated Patch, based on feedback.

4f5dcb4

dhruvachak approved these changes Apr 29, 2025

View reviewed changes

arsenm approved these changes May 2, 2025

View reviewed changes

arsenm reviewed May 2, 2025

View reviewed changes

llvm/test/CodeGen/AMDGPU/swdev504645-global-fold.ll Outdated Show resolved Hide resolved

fix whitespace error

89c7ff9

arsenm merged commit 9c9013f into llvm:main May 5, 2025
11 checks passed

isakhilesh deleted the updated_global_address branch May 30, 2025 16:18


		attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }

		!0 = !{!"DescriptorBuffer", i32 4, i32 8, i32 0, i32 0}

[AMDGPU] Handle MachineOperandType global address in SIFoldOperands. #135424

[AMDGPU] Handle MachineOperandType global address in SIFoldOperands. #135424

Uh oh!

Conversation

isakhilesh commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 11, 2025

Uh oh!

llvmbot commented Apr 11, 2025

Uh oh!

isakhilesh commented Apr 11, 2025

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

isakhilesh Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dhruvachak commented Apr 24, 2025

Uh oh!

arsenm commented Apr 24, 2025

Uh oh!

isakhilesh commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvachak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 5, 2025

Uh oh!

Uh oh!

isakhilesh commented Apr 11, 2025 •

edited

Loading

isakhilesh Apr 25, 2025 •

edited

Loading