This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][AsmParser] Refine parsing instruction operands.
ClosedPublic

Authored by kosarev on Nov 22 2022, 6:06 AM.

Details

Summary

Eliminates the need for working around optional and token operands being
mistakenly parsed as expressions.

Diff Detail

Event Timeline

kosarev created this revision.Nov 22 2022, 6:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 6:06 AM
kosarev requested review of this revision.Nov 22 2022, 6:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 6:06 AM

For operands that we know should never be considered valid expressions (such as id:... ones maybe?), this can be improved further by parsing them regardless of the instruction they are used in and their position. Also, for register operands we could call parseReg() directly from parseCustomOperand() instead of falling back to that after considering other possibilities in parseOperand().

llvm/utils/TableGen/AsmMatcherEmitter.cpp
1463–1470

Apart from allowing calling custom parsers for all operands, this also fixes marking possible positions of operands that come after optional operands.

foad added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
379

Why does only tfe need this?

kosarev updated this revision to Diff 477426.Nov 23 2022, 2:23 AM

Simplified parsing tfe and gds operands.

kosarev added inline comments.Nov 23 2022, 2:24 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
379

An earlier version of the patch used to parse tfe operands into both ImmTyTFEs and tokens, later changed to use ImmTyTFE in all cases for simplicity. I've removed this check and changed the rest of the code to employ parseNamedBit() for parsing hardcoded tfes. Same is done for gds, another operand we use in both the hardcoded and AsmOperand forms.

There's a similar situation with off, parsed into tokens while also having parseVReg32OrOff() adding them as ImmTyOffs. I'd prefer to address this separately along with other things like ImmTyIdxen being effectively unused, etc.

foad added inline comments.Nov 23 2022, 2:53 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
379

Thanks for the explanation.

dp accepted this revision.Nov 23 2022, 6:51 AM

LGTM. Nice improvements!

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
6045

I think it is not clear enough that the purpose of this break it to distinguish first and subsequent iterations.

6084

Is it possible to avoid this search on each iteration? Ideally, the code should accumulate cpol values in the loop, but store the result after exiting the loop.

Also, I'm not sure if it is possible to have 2 cpol operands separated by other operands. Maybe the search of a preceding cpol operand is not needed.

9119

I think this code is less readable and maintainable than the AMDGPUOptionalOperandTable.

This revision is now accepted and ready to land.Nov 23 2022, 6:51 AM
This revision was landed with ongoing or failed builds.Nov 24 2022, 3:19 AM
This revision was automatically updated to reflect the committed changes.
kosarev added inline comments.Nov 24 2022, 3:20 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
6045

I think the intention was to just make it stop on non-policy tokens.

6084

Good points. This clearly deserves a rewrite.

9119

The new code is a bit repetitive, but there are ways to deal with that. (I'm going to.)

foad added inline comments.Nov 24 2022, 11:35 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1821

Looks like this struct is unused now?

kosarev added inline comments.Nov 28 2022, 3:03 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1821