Page MenuHomePhabricator

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

Authored by kosarev on Mar 30 2023, 9:39 AM.

Details

Summary

Removes the need for the custom code in parseCustomOperand().

Diff Detail

Event Timeline

kosarev created this revision.Mar 30 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 9:39 AM
kosarev requested review of this revision.Mar 30 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 9:39 AM
foad added a comment.Mar 30 2023, 9:56 AM

Can you say a little bit more than "refine" (here and in other patches)? I can see you capitalised Sdwa and introduced a new SDWAOperand class but what's the bigger picture? I guess the win here is being able to remove some code from AMDGPUAsmParser.cpp?

Can you say a little bit more than "refine" (here and in other patches)? I can see you capitalised Sdwa and introduced a new SDWAOperand class but what's the bigger picture? I guess the win here is being able to remove some code from AMDGPUAsmParser.cpp?

Yes, same as with other similar patches this change removes the need for the custom code in parseCustomOperand(). Do you suggest the commit message to say that?

foad added a comment.Mar 30 2023, 10:58 AM

Can you say a little bit more than "refine" (here and in other patches)? I can see you capitalised Sdwa and introduced a new SDWAOperand class but what's the bigger picture? I guess the win here is being able to remove some code from AMDGPUAsmParser.cpp?

Yes, same as with other similar patches this change removes the need for the custom code in parseCustomOperand(). Do you suggest the commit message to say that?

Yes I would like the commit message to have a brief description of the point of the change.

kosarev updated this revision to Diff 509780.Mar 30 2023, 12:26 PM

Amended the commit message.

kosarev edited the summary of this revision. (Show Details)Mar 30 2023, 12:26 PM
rampitec added inline comments.Mar 30 2023, 12:28 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
9045–9048

Unrelated to the patch, but AsmParser violates 80 chars per line in many places. Maybe it needs to be just all clang-formatted.

foad accepted this revision.Mar 31 2023, 4:09 AM

LGTM, thanks.

This revision is now accepted and ready to land.Mar 31 2023, 4:09 AM
This revision was landed with ongoing or failed builds.Apr 18 2023, 7:43 AM
This revision was automatically updated to reflect the committed changes.