Page MenuHomePhabricator

[AMDGPU] Move const qualifier placement in AsmParser
ClosedPublic

Authored by lamb-j on Jan 20 2022, 9:32 AM.

Diff Detail

Event Timeline

lamb-j created this revision.Jan 20 2022, 9:32 AM
lamb-j requested review of this revision.Jan 20 2022, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 9:32 AM

Just move it

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1132–1133

Just move the const to the right place?

lamb-j updated this revision to Diff 401686.Jan 20 2022, 10:27 AM

Moved instead of removed const qualifiers

lamb-j retitled this revision from Remove unneeded const qualifiers from assembly parser to [AMDGPU] Move const qualifier placement in AsmParser.Jan 20 2022, 10:28 AM
lamb-j marked an inline comment as done.Jan 20 2022, 10:29 AM

While you are here, please drop the space between asterisk and Sym. I.e. 'const MCSymbol *Sym'.

lamb-j updated this revision to Diff 401706.EditedJan 20 2022, 11:00 AM

Fixing whitespace

This revision is now accepted and ready to land.Jan 20 2022, 11:05 AM
lamb-j added a comment.EditedJan 20 2022, 3:28 PM

@rampitec @arsenm If I'm understanding correctly, moving const to the left of "MCSymbol *"' declares the object as const instead of the pointer. In this case, we get a compilation error on the following line:

llvm-project/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:1133:9: error: 'this' argument to member function 'setVariableValue' has type 'const llvm::MCSymbol', but function is not marked const
    Sym->setVariableValue(MCConstantExpr::create(SgprIndexUnusedMin, *Ctx));

This error seems reasonable, as we're changing a member of Sym (Value). So in this case would we actually just want "MCSymbol *Sym" or "MCSymbol* const Sym"?

lamb-j requested review of this revision.Feb 1 2022, 9:25 AM
rampitec added inline comments.Feb 1 2022, 11:30 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1132–1133

80 symbols per line.

lamb-j updated this revision to Diff 405021.Feb 1 2022, 11:43 AM

Reformatting (clang-format suggestion)

lamb-j marked an inline comment as done.Feb 1 2022, 11:43 AM
rampitec accepted this revision.Feb 1 2022, 11:57 AM
This revision is now accepted and ready to land.Feb 1 2022, 11:57 AM
lamb-j updated this revision to Diff 405099.Feb 1 2022, 2:13 PM

Reverting code changes but keeping updated formatting to fix build error (see previous comment)

arsenm accepted this revision.Feb 1 2022, 4:20 PM
lamb-j updated this revision to Diff 405122.Feb 1 2022, 4:55 PM

Updating commit message

This revision was landed with ongoing or failed builds.Feb 1 2022, 6:01 PM
This revision was automatically updated to reflect the committed changes.