[AMDGPU][AsmParser] Some instructions are indistinguishable to AsmParser

[kosarev]

There are instructions in the AMDGPU backend that have identical mnemonics and operands:

$ grep 'BUFFER_ATOMIC_FCMPSWAP_OFFSET_.*gfx10' AMDGPUGenAsmMatcher.inc
  { 460 /* buffer_atomic_fcmpswap */, AMDGPU::BUFFER_ATOMIC_FCMPSWAP_OFFSET_RTN_gfx10, ConvertCustom_cvtMubufAtomic, AMFBS_isGFX6GFX7GFX10Plus_isGFX10Only, { MCK_AV_64, MCK_off, MCK_SReg_128, MCK_SCSrcB32, MCK_Offset, MCK_CPol }, },
  { 460 /* buffer_atomic_fcmpswap */, AMDGPU::BUFFER_ATOMIC_FCMPSWAP_OFFSET_gfx10, ConvertCustom_cvtMubufAtomic, AMFBS_isGFX6GFX7GFX10Plus_isGFX10Only, { MCK_AV_64, MCK_off, MCK_SReg_128, MCK_SCSrcB32, MCK_Offset, MCK_CPol }, },

Note that both the instructions have { MCK_AV_64, MCK_off, MCK_SReg_128, MCK_SCSrcB32, MCK_Offset, MCK_CPol } as their lists of operand kinds. This means that for a set of parsed operands that is suitable for these instructions as determined by calling their corresponding operand class predicates, of these two instructions AsmParser will select the one that is happened to match first. For instructions with identical mnemonics, operands and features the matching order is generally not defined.

Furthermore, where otherwise identical instructions have operands defined in TableGen as having different AsmOperandClasses, the matching order is determined by comparing the names of these AsmOperandClasses:

llvm-project/llvm/utils/TableGen/AsmMatcherEmitter.cpp

Line 364 in dda46b2

return ValueName < RHS.ValueName;

Particularly, when both the classes are anonymous, which is often the case in the AMDGPU backend, the matching order will therefore depend on their generated names. Generated names of anonymous TableGen entites have the form anonymous_<n>, where <n> is a number that depends on when during the work of the TableGen's internal machinery this entity was defined relative to other anonymous entities, see RecordKeeper::getNewAnonymousName(). This in turn means that the instruction matching order is generally susceptible to where in .td files used AsmOperandClasses have been defined.

As I side note, we seem to have special code in TableGen that checks specifically for ambiguous matchables (instructions):

llvm-project/llvm/utils/TableGen/AsmMatcherEmitter.cpp

Line 3237 in dda46b2

DEBUG_WITH_TYPE("ambiguous_instrs", {

However, because different anonymous operand classes are never considered equal, couldMatchAmbiguouslyWith() doesn't flag such cases.

Caught on an attempt to prepare an NFC patch involving adding an anonymous operand class, which led to failures on regression tests.

A subtask of #62629.

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

Author: Ivan Kosarev (kosarev)

There are instructions in the AMDGPU backend that have identical mnemonics and operands:

$ grep 'BUFFER_ATOMIC_FCMPSWAP_OFFSET_.*gfx10' AMDGPUGenAsmMatcher.inc
  { 460 /* buffer_atomic_fcmpswap */, AMDGPU::BUFFER_ATOMIC_FCMPSWAP_OFFSET_RTN_gfx10, ConvertCustom_cvtMubufAtomic, AMFBS_isGFX6GFX7GFX10Plus_isGFX10Only, { MCK_AV_64, MCK_off, MCK_SReg_128, MCK_SCSrcB32, MCK_Offset, MCK_CPol }, },
  { 460 /* buffer_atomic_fcmpswap */, AMDGPU::BUFFER_ATOMIC_FCMPSWAP_OFFSET_gfx10, ConvertCustom_cvtMubufAtomic, AMFBS_isGFX6GFX7GFX10Plus_isGFX10Only, { MCK_AV_64, MCK_off, MCK_SReg_128, MCK_SCSrcB32, MCK_Offset, MCK_CPol }, },

Note that both the instructions have { MCK_AV_64, MCK_off, MCK_SReg_128, MCK_SCSrcB32, MCK_Offset, MCK_CPol } as their lists of operand kinds. This means that for a set of parsed operands that is suitable for these instructions as determined by calling their corresponding operand class predicates, of these two instructions AsmParser will select the one that is happened to match first. For instructions with identical mnemonics, operands and features the matching order is generally not defined.

Furthermore, where otherwise identical instructions have operands defined in TableGen as having different AsmOperandClasses, the matching order is determined by comparing the names of these AsmOperandClasses:

llvm-project/llvm/utils/TableGen/AsmMatcherEmitter.cpp

Line 364 in dda46b2

return ValueName < RHS.ValueName;

Particularly, when both the classes are anonymous, which is often the case in the AMDGPU backend, the matching order will therefore depend on their generated names. Generated names of anonymous TableGen entites have the form anonymous_<n>, where <n> is a number that depends on when during the work of the TableGen's internal machinery this entity was defined relative to other anonymous entities, see RecordKeeper::getNewAnonymousName(). This in turn means that the instruction matching order is generally susceptible to where in .td files used AsmOperandClasses have been defined.

As I side note, we seem to have special code in TableGen that checks specifically for ambiguous matchables (instructions):

llvm-project/llvm/utils/TableGen/AsmMatcherEmitter.cpp

Line 3237 in dda46b2

DEBUG_WITH_TYPE("ambiguous_instrs", {

However, because different anonymous operand classes are never considered equal, couldMatchAmbiguouslyWith() doesn't flag such cases.

Caught on an attempt to prepare an NFC patch involving adding an anonymous operand class, which led to failures on regression tests.

A subtask of #62629.

[jayfoad]

1. What kind of problems occur when the matching order changes?
2. How do you think this should be fixed? In the BUFFER_ATOMIC_FCMPSWAP_OFFSET case, I guess the intention was that the RTN form would only match when the glc bit is set, and the non-RTN form would only match when glc is not set.

[kosarev]

1. What kind of problems occur when the matching order changes?
2. How do you think this should be fixed? In the BUFFER_ATOMIC_FCMPSWAP_OFFSET case, I guess the intention was that the RTN form would only match when the glc bit is set, and the non-RTN form would only match when glc is not set.

For instructions that look identical to the AsmParser machinery we may end up matching the wrong instruction, which is what actually happened as I was trying to fix the predicates for DefaultOperands, such as CPol_0 and CPol_GLC1, to take their expected values into account. This required giving them their own special operand classes, because this is where predicates are specified. But since these new classes have different names, this changed the matching order.

(I think I was wrong saying it above that it is the names of the classes themselves that matter. I was investigating this months ago and now that I refresh my memory, I think it's the Name fields of the classes actually, though this doesn't change much in the analysis of the problem.)

Regarding means to fix this, maybe we could start with using the HasPositionOrder field of the TableGen's Instruction class to mitigate the problem with floating matching orders. That would allow us to then fix DefaultOperand predicates and likely make life a bit easier working on other AsmParser tasks involving adding new operand classes. After that we could probably think about fixing the ordering logic in such a way that would not require relying on HasPositionOrder and do the expected thing automatically. Then once the ordering logic is in order, I think it would be nice to improve the ambiguity checks to catch more, preferably all, problems of this sort. And ultimately, I think it would be fair to say that our goal is a simple, reliable and intuitive interface to AsmParser that would eliminate the flow of mistakes and frustration about what and how needs to be done when comes to adding new or changing existing instruction operands -- that's what #62629 attempts to provide.

That apart, it's also possible that we will have to teach the AsmMatcherEmitter backend how to deal with optional operands. It seems it currently just ignores their existence. I don't have any concrete reproducers, but suspect that fixing the ordering logic will require taking them into account.

[kosarev]

This reproduces the problem. Supposed to be an NFC, but causes failures on check-llvm-mc-amdgpu. Tested on 561fcf5.

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.td b/llvm/lib/Target/AMDGPU/SIInstrInfo.td
index b0b91d831718..7301deb597ae 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.td
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.td
@@ -1038,10 +1038,19 @@ class NamedBitOperand<string Id, string Name = NAME>
 
 class DefaultOperand<CustomOperand Op, int Value>
   : OperandWithDefaultOps<Op.Type, (ops (Op.Type Value))>,
-    CustomOperandProps<1, Op.ParserMatchClass.Name> {
+    CustomOperandProps<1> {
+  let PredicateMethod = Op.ParserMatchClass.PredicateMethod;
+  // Give this operand a class that is identical to that of Op.
+  let ImmTy = Op.ImmTy;
   let PredicateMethod = Op.ParserMatchClass.PredicateMethod;
   let ParserMethod = Op.ParserMatchClass.ParserMethod;
+  let DefaultValue = Op.DefaultValue;
   let PrintMethod = Op.PrintMethod;
+
+  // check-llvm-mc-amdgpu tests fail unless we use the class of Op,
+  // despite both classes being identical in all their attributes
+  // (except the Name fields, which should not matter).
+  // let ParserMatchClass = Op.ParserMatchClass;
 }
 
 class SDWAOperand<string Id, string Name = NAME>

$ ninja check-llvm-mc-amdgpu
...
Failed Tests (17):
  LLVM :: MC/AMDGPU/expressions.s
  LLVM :: MC/AMDGPU/gfx1030_new.s
  LLVM :: MC/AMDGPU/gfx10_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx10_asm_smem.s
  LLVM :: MC/AMDGPU/gfx11_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx11_asm_mubuf_alias.s
  LLVM :: MC/AMDGPU/gfx7_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx8_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx90a_asm_features.s
  LLVM :: MC/AMDGPU/gfx90a_ldst_acc.s
  LLVM :: MC/AMDGPU/gfx940_asm_features.s
  LLVM :: MC/AMDGPU/gfx9_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx9_asm_smem.s
  LLVM :: MC/AMDGPU/mubuf-gfx10.s
  LLVM :: MC/AMDGPU/mubuf.s
  LLVM :: MC/AMDGPU/smem.s
  LLVM :: MC/AMDGPU/trap.s


Testing Time: 1.62s
  Passed           : 326
  Expectedly Failed:   1
  Failed           :  17