[PATCH] D137549: [AsmParser] Match mandatory operands following optional operands.

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp added a comment.

Overall it is a nice idea, but I'm not sure how useful it would be for non AMDGPU backends. I suspect that most backends do not mix optional and mandatory operands (but I may be wrong).

Anyway, I think that the change should be split to tablegen and AMDGPU parts - they probably need different reviewers.

A description of your idea with a rationale would be useful (especially for reviewers supporting non AMDGPU backends).



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:4585
 
 bool AMDGPUAsmParser::validateLdsDMA(uint64_t Enc, const MCInst &Inst,
                                      const OperandVector &Operands,
----------------
It looks like this function is no longer necessary: it always returns true.


================
Comment at: llvm/utils/TableGen/AsmMatcherEmitter.cpp:3657
      << ", ActualIdx = " << (HasMnemonicFirst ? "1" : "SIndex")
      << "; FormalIdx != " << MaxNumOperands << "; ++FormalIdx) {\n";
   OS << "      auto Formal = "
----------------
For AMDGPU the generated code is:

    ...
    for (unsigned FormalIdx = 0, ActualIdx = 1; FormalIdx != 26; ++FormalIdx) {
      auto Formal = static_cast<MatchClassKind>(it->Classes[FormalIdx]);

      if (ActualIdx >= Operands.size()) {
        OptionalOperandsMask.set(FormalIdx, 26);
        OperandsValid = (Formal == InvalidMatchClass) || isSubclass(Formal, OptionalMatchClass);
        if (OperandsValid) {
          ++ActualIdx;
          continue;
        }
        ErrorInfo = ActualIdx;
        break;
      }
      ...
    }
    ...

It looks like each successful match will have to iterate over 26 formal operand classes to make sure there are no mandatory operands. Isn't it wasteful?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137549