This is an archive of the discontinued LLVM Phabricator instance.

[amdgpu] Add 64-bit PC support when expanding unconditional branches.
ClosedPublic

Authored by hliao on Jul 21 2021, 7:34 AM.

Diff Detail

Event Timeline

hliao created this revision.Jul 21 2021, 7:34 AM
hliao requested review of this revision.Jul 21 2021, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2021, 7:34 AM
hliao added a comment.Jul 21 2021, 7:42 AM

This change revises the offset calculation, not only for 64-bit support but also for the removal of the assumption in AMDGPUMCInstLower::getLongBranchBlockExpr in the preparation to handle the case where there's no scavenged register when expanding an unconditional branch. In that case, s_getpc follows a code sequence spilling an SGPR pair. But, that sequence has a variable code length.

hliao updated this revision to Diff 360481.Jul 21 2021, 8:48 AM

Fix pre-merge checks

Do any of these tests stress object emission?

Should also make sure we have some MC tests to make sure these new expressions parse

hliao updated this revision to Diff 360684.Jul 21 2021, 7:10 PM

Enhance MC test on the new expression patterns.

hliao updated this revision to Diff 360810.Jul 22 2021, 7:34 AM

Rebase and test another MC encoding test.

  • llvm/test/MC/AMDGPU/expressions.s adds the assembly parsing test
  • llvm/test/MC/AMDGPU/offset-expr.s adds the encoding test
hliao added a comment.Jul 22 2021, 7:35 AM

Do any of these tests stress object emission?

Should also make sure we have some MC tests to make sure these new expressions parse

two MC tests are added

arsenm added inline comments.Jul 22 2021, 11:10 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
2307

This is supposed to return the number of bytes added, not necessarily the size of the block. At least should assert the block is empty, which I think is always the case

llvm/test/CodeGen/AMDGPU/branch-relaxation-debug-info.mir
12

It would be better if this mask was printed as hex, but I guess there’s no direct control over this

hliao added inline comments.Jul 22 2021, 12:33 PM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
2307

the assertion on the empty block is already added on L2217. That's how I was motivated to get this change.

llvm/test/CodeGen/AMDGPU/branch-relaxation-debug-info.mir
12

yes, there is no control on that as the readability depends on the value itself. People may prefer small values in decimal but larger one or special values in hex. But, the definition on small, large, or special values is quite diverse.

hliao added inline comments.Jul 22 2021, 12:34 PM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
2217

the assertion on the empty block is already there

hliao updated this revision to Diff 361736.Jul 26 2021, 11:21 AM

Rebase and kindly ping for review.

arsenm accepted this revision.Jul 26 2021, 11:35 AM
This revision is now accepted and ready to land.Jul 26 2021, 11:35 AM
This revision was landed with ongoing or failed builds.Jul 26 2021, 11:50 AM
This revision was automatically updated to reflect the committed changes.