This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Clean up DPP bound_ctrl handling
ClosedPublic

Authored by rovka on Apr 26 2023, 6:12 AM.

Details

Reviewers
dp
foad
Group Reviewers
Restricted Project
Commits
rG7591a7b6ea5a: [AMDGPU][MC] Clean up DPP bound_ctrl handling
Summary

At the moment, we set the BC bit in DPP for both bound_ctrl:0 and
bound_ctrl:1, for compatibility with sp3 (see PR35397). However, this
hack is only needed for GFX8. For newer GFXs, sp3 behaves as expected,
i.e. it sets the bit when bound_ctrl:1 and clears it when bound_ctrl:0.

This patch updates LLVM to do the same for GFX11 or newer. We preserve
the current behaviour for GFX9 and GFX10 so we don't break any existing
code.

Diff Detail

Event Timeline

rovka created this revision.Apr 26 2023, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 6:12 AM
rovka requested review of this revision.Apr 26 2023, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 6:12 AM
rovka added reviewers: Restricted Project, dp.Apr 26 2023, 6:14 AM
kosarev added inline comments.Apr 26 2023, 6:36 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.td
1276

What if instead pass the AMDGPUAsmParser object to the conversion handler so ConvertDppBoundCtrl() could call isGFX9Plus() itself?

OperandMatchResultTy
parseIntWithPrefix(const char *Prefix, OperandVector &Operands,
                   AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyNone,
                   bool (*ConvertResult)(int64_t &, const AMDGPUAsmParser &) = nullptr);
foad added a comment.Apr 26 2023, 7:06 AM

[AMDGPU][MC] Clean up DPP bound_crtl handling

Typo "ctrl"

dp added a comment.Apr 26 2023, 7:23 AM

IMO this change should be limited to GFX11 to preserve compatibility with existing assembly code.
Documentation should be corrected as well - see https://llvm.org/docs/AMDGPUModifierSyntax.html#bound-ctrl

foad added a comment.Apr 26 2023, 7:53 AM
In D149254#4298864, @dp wrote:

IMO this change should be limited to GFX11 to preserve compatibility with existing assembly code.

Why GFX11? Do you mean because it was the first architecture introduced *after* you fixed the disassembler to output bound_ctrl:1 instead of bound_ctrl:0 in D97048?

dp added a comment.Apr 26 2023, 8:28 AM
In D149254#4298864, @dp wrote:

IMO this change should be limited to GFX11 to preserve compatibility with existing assembly code.

Why GFX11? Do you mean because it was the first architecture introduced *after* you fixed the disassembler to output bound_ctrl:1 instead of bound_ctrl:0 in D97048?

This patch only affects assembler and the changes break compatibility with existing code.
I think there is a lot of asm code written for GFX9 and GFX10 and the code may theoretically use bound_ctrl:0 assuming the same behavior as bound_ctrl:1. This is less an issue with GFX11.

rovka updated this revision to Diff 517508.Apr 27 2023, 3:48 AM
  • Updated the docs
  • Switched to GFX11+
  • Refactored the code a bit: made convertDppBoundCtrl a member function and

updated parseIntWithPrefix to take a std::function so we can pass a lambda
binding this into it. This isn't exactly what Ivan suggested but it's in the
same spirit and allows us to keep the same interface for the callback (and use
the existing standalone functions where we don't need the AMDGPUAsmParser object)

rovka retitled this revision from [AMDGPU][MC] Clean up DPP bound_crtl handling to [AMDGPU][MC] Clean up DPP bound_ctrl handling.Apr 27 2023, 4:03 AM
rovka edited the summary of this revision. (Show Details)
kosarev added inline comments.Apr 27 2023, 5:26 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.td
1276

Can we now just do NamedIntOperand<i1, "bound_ctrl", "DppBoundCtrl", "[this](int64_t &BC) -> bool { return convertDppBoundCtrl(BC); }">;?

It is still a NamedIntOperand, and there should be no need to make it look otherwise.

rovka updated this revision to Diff 517826.Apr 28 2023, 12:36 AM

Updated based on Ivan's comment (good point, thanks!).

The SIInstrInfo.td changes LGTM, thanks.

rovka added a comment.May 8 2023, 1:13 AM

@dp @foad Do you have any further comments or is this ok to commit?

dp accepted this revision.May 8 2023, 11:43 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 8 2023, 11:43 AM
foad accepted this revision.May 9 2023, 3:40 AM
In D149254#4299082, @dp wrote:
In D149254#4298864, @dp wrote:

IMO this change should be limited to GFX11 to preserve compatibility with existing assembly code.

Why GFX11? Do you mean because it was the first architecture introduced *after* you fixed the disassembler to output bound_ctrl:1 instead of bound_ctrl:0 in D97048?

This patch only affects assembler and the changes break compatibility with existing code.
I think there is a lot of asm code written for GFX9 and GFX10 and the code may theoretically use bound_ctrl:0 assuming the same behavior as bound_ctrl:1. This is less an issue with GFX11.

OK, I take that to mean that it's an arbitrary stake in the ground :)

No objection to the patch.

This revision was landed with ongoing or failed builds.May 10 2023, 2:41 AM
This revision was automatically updated to reflect the committed changes.