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

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp marked an inline comment as done.
dp added a comment.

Hello @sdesmalen,

I'm sorry for not trying your patch earlier. I thought that it only fixed problems with operand matcher and I had a feeling that most problems were located in instruction matcher.
Well, I finally tried your patch. It did not fix all issues, but for on-liner it is really good!
There are several lit tests created specifically to test unsupported instructions. Below is a summary of results with and without your patch.

  	Test name              number      failed       failed      improvement
                                of tests      base    (sdesmalen fix)   vs base
  	gfx7_unsupported.s      1044        502           316           37%
  	gfx8_unsupported.s       599        129            35           73%
  	gfx9_unsupported.s       342         67            24           64%
  	gfx10_unsupported.s      720        345           224           35%

Enclosed is the smallest of these files. To simplify analysis, tests have been corrected as follows:

- passed tests removed;
- failed tests updated so that the tests pass with your patch;
- a header in each section describes an expected error message.

In other words the enclosed tests show actual error messages displayed by assembler after your patch has been applied.
F13116240: gfx9_unsupported_failed.s <https://reviews.llvm.org/F13116240>

I should also note that AMDGPU has several instruction variants identified by mnemonic suffices (_e32, _e64, _dpp, _sdwa). There are cases when the base instruction is supported but a variant is not. The proposed extension allows easy identification of such cases to provide appropriate diagnostic messages. The enclosed lit test has several examples at the end of the file.



================
Comment at: llvm/utils/TableGen/AsmMatcherEmitter.cpp:3977
+
+  emitMnemonicChecker(OS, Target, VariantCount,
+                      HasMnemonicFirst, HasMnemonicAliases);
----------------
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.



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

https://reviews.llvm.org/D88214