Page MenuHomePhabricator

[AMDGPU][MC] Correct handling of mandatory literals
ClosedPublic

Authored by dp on Nov 24 2022, 5:00 AM.

Details

Summary

Mandatory literals are incorrectly handled by the parser.

When the value of a mandatory literal is in the range of inline constants, it is not accounted for as a literal. As a result, assembler may accept instructions with several different literals. See this bug for more information.

Diff Detail

Event Timeline

dp created this revision.Nov 24 2022, 5:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 5:00 AM
dp requested review of this revision.Nov 24 2022, 5:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 5:00 AM
Joe_Nash added inline comments.Nov 28 2022, 11:31 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3382

This looks a bit weird to me because isSISrcOperand used to be true for KImm, until I created the mandatory literal logic. I'm not sure what the desired semantics of isSISrcOperand are. Can you try making isSISrcOperand used to be true for KImm and seeing if there is any fallout?

dp added inline comments.Nov 29 2022, 10:00 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3382

isSISrcOperand does return true for KImm and seems it always did. I just added a limitation on KImm operands - they can no longer be interpreted as inline constants.

Before the mandatory literal logic was introduced, this limitation on KImm operands was unnecessary because only one literal was allowed for affected instructions.

Could you describe your concern in more detail?

dp marked an inline comment as done.Nov 29 2022, 11:19 AM
dp added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3382

Are you suggesting to see if it is feasible to make isSISrcOperand return false for KImm?

Joe_Nash added inline comments.Nov 29 2022, 11:48 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3382

Are you suggesting to see if it is feasible to make isSISrcOperand return false for KImm?

Yes.

Sorry, I said the reverse of what I meant.

dp updated this revision to Diff 479230.Dec 1 2022, 3:45 AM

Rebase; update tests to add error line numbers.

dp added inline comments.Dec 1 2022, 4:50 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3382

I prepared a separate change on top of this patch to see how this can be done. Please take a look. I'm not sure if this would be that useful.

https://reviews.llvm.org/D139101

Joe_Nash accepted this revision.Dec 1 2022, 6:48 AM

Ok, apart from that comment LGTM.

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

Thanks for trying that. I agree it doesn't look too useful.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
1129–1130

Can you make this comment more precise?

This revision is now accepted and ready to land.Dec 1 2022, 6:48 AM
dp updated this revision to Diff 479295.Dec 1 2022, 7:46 AM

Update comment describing isSISrcOperand.

Joe_Nash accepted this revision.Dec 1 2022, 7:47 AM
This revision was landed with ongoing or failed builds.Dec 5 2022, 5:24 AM
This revision was automatically updated to reflect the committed changes.
cfang added a subscriber: cfang.Thu, Jan 12, 4:30 PM
cfang added inline comments.
llvm/lib/Target/AMDGPU/SIDefines.h
207

These two definitions caused an issue. it makes OPERAND_INPUT_MODS = OPERAND_KIMM16 + 1, which is actually
OPERAND_REG_INLINE_AC_INT16 ( next enumerator). As a result, two unrelated enumerators have the same value.

I create a patch to address the issue -- we can put uninitialized enumerators together.
https://reviews.llvm.org/D141643