[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

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

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

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

@@ -1161,6 +1161,10 @@ void SIFoldOperandsImpl::foldOperand(

        if (OpToFold.isImm())
          UseMI->getOperand(1).ChangeToImmediate(OpToFold.getImm());
+        else if (OpToFold.isGlobal())


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

isakhilesh commented Apr 11, 2025 •

edited

Loading

github-actions bot commented Apr 11, 2025

llvmbot commented Apr 11, 2025

isakhilesh commented Apr 11, 2025

arsenm left a comment

arsenm Apr 24, 2025

dhruvachak Apr 24, 2025

arsenm Apr 24, 2025

isakhilesh Apr 25, 2025 •

edited

Loading

arsenm Apr 25, 2025

isakhilesh Apr 26, 2025

dhruvachak Apr 26, 2025

arsenm Apr 26, 2025

dhruvachak commented Apr 24, 2025

arsenm commented Apr 24, 2025

isakhilesh commented Apr 25, 2025

arsenm Apr 25, 2025

arsenm Apr 25, 2025

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

Are you sure you want to change the base?

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

Conversation

isakhilesh commented Apr 11, 2025 • edited Loading

github-actions bot commented Apr 11, 2025

llvmbot commented Apr 11, 2025

isakhilesh commented Apr 11, 2025

arsenm 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

isakhilesh Apr 25, 2025 • edited Loading

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

dhruvachak commented Apr 24, 2025

arsenm commented Apr 24, 2025

isakhilesh commented Apr 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isakhilesh commented Apr 11, 2025 •

edited

Loading

isakhilesh Apr 25, 2025 •

edited

Loading