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

Sat Oct 3 11:32:34 PDT 2020

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

Hello @sdesmalen,

> Just to make sure I understand this correctly though. These instructions are valid for at least some invocation of feature flags, <...>

The tests we are discussing include:

- Pure-negative cases which are not valid for any combinations of instruction variants and feature flags;
- Tests which are valid for at least one AMDGPU subrarget.

In case of gfx9_unsupported.s assembler with your fix did not produce expected results for 24 tests. 12 tests out of these 24 tests are valid for at least one AMDGPU subtarget. Remaining 12 tests are pure-negative. So your fix is even more efficient if we exclude pure-negative tests from consideration.

Enclosed is an updated version of tests with pure-negative cases removed: F13140477: gfx9_unsupported_failed2.s <https://reviews.llvm.org/F13140477>

> ... 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.

I'm not sure about //"not be legal for any feature set"//.
I further analyzed 12 failures where your fix did not succeed. These 12 cases may be reduced to the following 2 cases:

1. A mismatch in `MatchOperandParserImpl`. One of operands has a custom parser and the parser explicitly checks a feature and returns `MatchOperand_NoMatch` because the feature is not supported.
2. A mismatch in `MatchInstructionImpl`. This case is more interesting because both gfx9 and gfx10 use the same variantId but due to different set of features the instruction is valid for gfx10 bit illegal (unsupported) for gfx9.  `MatchInstructionImpl` is unable to find a suitable match for gfx9. Despite feature mismatch with all found mnemonics, it does not return `Match_MissingFeature`, for some reason `Match_InvalidOperand` is preferred.

> 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?

Correct.

Thank you!

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

https://reviews.llvm.org/D88214