[AMDGPU][MC] Correct handling of mandatory literals

[dp]

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.

[Joe_Nash]

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]

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]

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]

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]

Rebase; update tests to add error line numbers.

[dp]

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]

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?

[dp]

Update comment describing isSISrcOperand.

[cfang]

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