[PATCH] D88214: [TableGen] Added a function to identify unsupported opcodes

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: pengfei.

Hi @dp, thanks for your detailed reply!

In D88214#2306928 <https://reviews.llvm.org/D88214#2306928>, @dp wrote:

> In other words the enclosed tests show actual error messages displayed by assembler after your patch has been applied.

Just to make sure I understand this correctly though. These instructions are valid for at least some invocation of feature flags, but depending on which table `MatchInstructionImpl` is looking in (VariantID), it may not be legal for any feature set and thus lead to a 'illegal operand' diagnostic. But for AMDGPU that could have also been deduced from the mnemonic alone, hence the preference to check that before calling `MatchInstructionImpl`. Is that a correct?

In any case there would be no downside to generating this checker-function, even if it isn't used by other targets, so I'm happy to LGTM based on your analysis!



================
Comment at: llvm/utils/TableGen/AsmMatcherEmitter.cpp:3977
+
+  emitMnemonicChecker(OS, Target, VariantCount,
+                      HasMnemonicFirst, HasMnemonicAliases);
----------------
dp wrote:
> sdesmalen wrote:
> > Should this be called by `MatchInstructionImpl` so that this feature comes for free for all AsmParsers?
> I'm not sure.
> 
> First, your fix may be quite sufficient for many backends. 
> 
> Second, I considered this (proposed) function as a helper for constructing higher-level utilities for each backend. For example, AMDGPU uses this function to identify both unsupported instructions and unsupported instruction variants. This may or may not be needed for other backends.
> 
> Third, a single call to `MatchInstructionImpl` is not sufficient to trigger an error. `MatchInstructionImpl` only handles a specific set of features and one instruction variant. A backend may repeatedly call this function to search the mnemonic in different variant tables. But identification of unsupported opcodes may require more than that. For AMDGPU we have to search all variants and ignore feature bits to make sure the mnemonic in question is supported in some variant or feature set. But variant/feature search rules may be different for other backends.
> 
> So I think the new proposed functionality should be available as a separate function called either before first call of `MatchInstructionImpl` or after all calls to `MatchInstructionImpl` failed to locate a match.
> 
`MatchInstructionImpl` will never return with `Match_Success` if we can prove that none of the feature bits enables the mnemonic, and so this could be a cheap exit from that function. But it would also slow down the assembler for all valid instructions as it would always need to check that table first. So I agree, happy to leave it as-is, the existing mechanism is probably sufficient for most use-cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88214/new/

https://reviews.llvm.org/D88214