This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Take care of "tied" operand when removeOperand
ClosedPublic

Authored by cfang on Jul 25 2022, 7:06 PM.

Details

Summary

Flat scratch load of D16 type by default has tied vdst_in operand (with vdst). This should be taken
care of at the time of "removeOperand" in eliminateFrameIndex. Otherwise we will hit an assert saying
"Cannot move tied operands". This patch unties vdst_in before the move, and retie it with vdst afterwards.

Diff Detail

Event Timeline

cfang created this revision.Jul 25 2022, 7:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 7:06 PM
cfang requested review of this revision.Jul 25 2022, 7:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 7:06 PM
Herald added a subscriber: wdng. · View Herald Transcript
foad added inline comments.Jul 26 2022, 2:33 AM
llvm/test/CodeGen/AMDGPU/tied-operand.ll
1 ↗(On Diff #447536)

Can you add this to frame-index-elimination.ll instead of creating a new file? Or at least give the new file a more descriptive name, maybe frame-index-elimination-tied-operand.ll?

cfang marked an inline comment as done.Jul 26 2022, 10:55 AM
cfang added inline comments.
llvm/test/CodeGen/AMDGPU/tied-operand.ll
1 ↗(On Diff #447536)

Rename to frame-index-elimination-tied-operand.ll. Using a new file to avoid unnecessary checks.

cfang updated this revision to Diff 447762.Jul 26 2022, 10:57 AM
cfang marked an inline comment as done.

Rename the LIT test to frame-index-elimination-tied-operand.ll as suggested.

I'd also like a mir test

llvm/test/CodeGen/AMDGPU/frame-index-elimination-tied-operand.ll
2 ↗(On Diff #447762)

Can you merge this test with one of the existing frame elimination tests?

12–13 ↗(On Diff #447762)

Avoid using undefs in this test

cfang updated this revision to Diff 448169.Jul 27 2022, 2:53 PM

Add a mir test, and remove the use of undef in the test.

cfang marked an inline comment as done.Jul 27 2022, 2:57 PM
cfang added inline comments.
llvm/test/CodeGen/AMDGPU/frame-index-elimination-tied-operand.ll
2 ↗(On Diff #447762)

There is a difficulty in the merge. This test is only for gfx1100, and we should not check for any other targets. And the existing tests of frame index elimination never check for gfx1100. It is beyond this work to do additional checks for gfx1100 for other works in frame index elimination.

cfang updated this revision to Diff 448214.Jul 27 2022, 5:08 PM

Merge the new test into frame-index-elimination.ll

arsenm accepted this revision.Jul 28 2022, 4:47 PM
arsenm added inline comments.
llvm/test/CodeGen/AMDGPU/frame-index-elimination-tied-operand.mir
3

Should add gfx11 to the test name?

llvm/test/CodeGen/AMDGPU/frame-index-elimination.ll
4

Would be better to have full checks for all targets on all functions

This revision is now accepted and ready to land.Jul 28 2022, 4:47 PM
This revision was landed with ongoing or failed builds.Jul 28 2022, 5:32 PM
This revision was automatically updated to reflect the committed changes.