This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] gfx11 VOPC instructions
ClosedPublic

Authored by Joe_Nash on Jun 3 2022, 12:02 PM.

Details

Reviewers
foad
rampitec
kzhuravl
dp
Group Reviewers
Restricted Project
Commits
rGbe1082c6d54d: [AMDGPU] gfx11 VOPC instructions
Summary

Supports encoding existing instrutions on gfx11 and MC support for the new VOPC
dpp instructions.

Patch 19/N for upstreaming of AMDGPU gfx11 architecture

Depends on D126978

Diff Detail

Event Timeline

Joe_Nash created this revision.Jun 3 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 12:02 PM
Joe_Nash requested review of this revision.Jun 3 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 12:02 PM
Joe_Nash added reviewers: foad, rampitec, Restricted Project, kzhuravl, dp.Jun 3 2022, 12:06 PM
rampitec added inline comments.Jun 3 2022, 12:28 PM
llvm/test/CodeGen/AMDGPU/move-load-addr-to-valu.mir
22 ↗(On Diff #434098)

Why did it change? This is unexpected.

dp added inline comments.Jun 7 2022, 8:46 AM
llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
628

The name of the method is somewhat unexpected - what does "Mods" mean? Maybe "needsImpliedVcc" would be better?

llvm/lib/Target/AMDGPU/VOPCInstructions.td
63

Indent

65

Ditto

67–68

Ditto

1028

Most of the code has no spaces around '-' when specifying bit ranges, but the code here and below is different. We should follow the style of existing code. https://llvm.org/docs/CodingStandards.html#golden-rule.

1070

Extra spaces.

1126

Indent

1133

Ditto

1193–1196

Indent is correct but should be increased for better readability (or ':' may be moved to the next line).

1208

Indent

1407–1408

Indent

1492–1493

Extra line

1679

Extra line

llvm/test/MC/AMDGPU/gfx11_asm_vopc_dpp.s
119

Extra line

122

IMO the tests would look cleaner with uniform comments token ('//') and uniform (3-line) separator.

Joe_Nash added inline comments.Jun 7 2022, 8:51 AM
llvm/test/CodeGen/AMDGPU/move-load-addr-to-valu.mir
22 ↗(On Diff #434098)

These are the only changes to the tablegen record.
field bit VOPC = 0; -> field bit VOPC = 1;
bits<64> TSFlags (VOPC bit is now set)

There is no difference in the SIFixSGPRCopies output. It seems to me as if when the instruction is being constructed it is now commuted. Since this instruction should commute, do you think this is a bad change?

foad added inline comments.Jun 7 2022, 9:20 AM
llvm/lib/Target/AMDGPU/VOPCInstructions.td
1028

Or we should switch to Inst{8...0} syntax. https://llvm.org/docs/TableGen/ProgRef.html says "The use of hyphen as the range punctuation is deprecated."

foad added inline comments.Jun 7 2022, 9:57 AM
llvm/test/CodeGen/AMDGPU/move-load-addr-to-valu.mir
22 ↗(On Diff #434098)

V_CMP_NE_U64_e64 is now marked as both VOPC and VOP3, which means that SIInstrInfo::legalizeOperands now sends it to legalizeOperandsVOP2 instead of legalizeOperandsVOP3. legalizeOperandsVOP2 commutes the operands.

This all feels slightly wrong. VOPC is an encoding, and V_CMP_NE_U64_e64 uses the VOP3 encoding not the VOPC encoding, so it should not be marked as VOPC - should it??

rampitec added inline comments.Jun 7 2022, 12:09 PM
llvm/test/CodeGen/AMDGPU/move-load-addr-to-valu.mir
22 ↗(On Diff #434098)

Interesting. This sounds like it needs a revision of every single place we are using isVOPC()...

Joe_Nash marked 17 inline comments as done.Jun 9 2022, 7:06 AM
Joe_Nash added inline comments.
llvm/lib/Target/AMDGPU/VOPCInstructions.td
1028

I will make it uniform with existing code. It seems like the whole Target could be converted to the new version with a text substitution in the future.

llvm/test/CodeGen/AMDGPU/move-load-addr-to-valu.mir
22 ↗(On Diff #434098)

I will remove let VOPC = 1 from the 64 bit instructions so isVOP3 and isVOPC remain mutually exclusive

Joe_Nash updated this revision to Diff 435533.Jun 9 2022, 7:06 AM
Joe_Nash marked an inline comment as done.

removed let VOPC =1 from VOP3 instructions, addressed formatting concerns, reverted test change

Joe_Nash updated this revision to Diff 435615.Jun 9 2022, 11:01 AM

change comment token in test

Joe_Nash updated this revision to Diff 435616.Jun 9 2022, 11:02 AM

remove whitespace

Joe_Nash marked an inline comment as done.Jun 9 2022, 11:04 AM

Addressed all comments. PTAL

llvm/test/MC/AMDGPU/gfx11_asm_vopc_dpp.s
122

I have changed to a uniform ('//') comment token. The separators are hierarchical, so I would prefer to leave them heterogeneous.

dp added a comment.Jun 9 2022, 11:45 AM

MC layer changes look good to me, thanks!

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
629

Indent

llvm/lib/Target/AMDGPU/VOPCInstructions.td
1030

There are 9 more lines where bit range is specified with spaces around "-".

Joe_Nash updated this revision to Diff 435624.Jun 9 2022, 11:54 AM

shrunk whitespace around -, alignment

rampitec added inline comments.Jun 9 2022, 11:56 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1765

Could you please reformat it to 8 chars max?

1765

I mean 80 of course.

Joe_Nash updated this revision to Diff 435627.Jun 9 2022, 12:00 PM

asmparser defines to 80 chars length

rampitec accepted this revision.Jun 9 2022, 12:02 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 9 2022, 12:02 PM
This revision was landed with ongoing or failed builds.Jun 9 2022, 12:50 PM
This revision was automatically updated to reflect the committed changes.