Page MenuHomePhabricator

[AsmParser] Match mandatory operands following optional operands.
ClosedPublic

Authored by kosarev on Nov 7 2022, 5:58 AM.

Details

Summary

Currently, the asm parser stops matching instruction operands as soon as the first optional operand is encountered. This leads to the need for custom checks on missing mandatory operands that come after optional operands.

The patch changes the parser to always match all optional and mandatory instruction operands, thus making the custom checks unnecessary. This is particularly useful for the AMDGPU backend where we have numerous optional instruction modifiers.

Diff Detail

Event Timeline

kosarev created this revision.Nov 7 2022, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2022, 5:58 AM
kosarev requested review of this revision.Nov 7 2022, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2022, 5:58 AM

I think this could also make the hack added in https://reviews.llvm.org/D41598 unnecessary but it doesn't because of https://reviews.llvm.org/D20527 not respecting types of optional parameters, meaning mandatory parameters that follow optional ones may be off their positions and fail to match, which is a problem of its own.

dp added a comment.Nov 7 2022, 10:17 AM

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

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4585 ↗(On Diff #473643)

It looks like this function is no longer necessary: it always returns true.

llvm/utils/TableGen/AsmMatcherEmitter.cpp
3657

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?

dp added inline comments.Nov 7 2022, 11:08 AM
llvm/test/MC/AMDGPU/gfx10_err_pos.s
7

Could you also rename the section?

968–969

This and the following test should be moved to another section.

llvm/test/MC/AMDGPU/gfx9_err_pos.s
185

Could you also rename the section?

kosarev updated this revision to Diff 473970.Nov 8 2022, 5:55 AM

Addressed review feedback.

  • Removed AMDGPU-specific changes.
  • Added some richer description of the idea and reasoning.
  • The generated code changed to stop after the last formal.
  • Tests updated as suggested.
kosarev marked 4 inline comments as done.Nov 8 2022, 5:57 AM

Regarding how useful it might be for other backends, feels like matching all operands may actually be that least confusing default behaviour to be expected by default?

The generated code now reads:

if (ActualIdx >= Operands.size()) {
  DEBUG_WITH_TYPE("asm-matcher", dbgs() << "actual operand index out of range\n");
  if (Formal == InvalidMatchClass) {
    OptionalOperandsMask.set(FormalIdx, 26);
    break;
  }
  if (isSubclass(Formal, OptionalMatchClass)) {
    OptionalOperandsMask.set(FormalIdx);
    continue;
  }
  OperandsValid = false;
  ErrorInfo = ActualIdx;
  break;
}
kosarev edited the summary of this revision. (Show Details)Nov 8 2022, 5:58 AM
kosarev edited the summary of this revision. (Show Details)Nov 8 2022, 5:59 AM
kosarev marked an inline comment as done.Nov 8 2022, 6:21 AM

AMDGPU changes went to https://reviews.llvm.org/D137638.

kosarev updated this revision to Diff 473992.Nov 8 2022, 7:05 AM

Remove an unwanted formatting change.

dp accepted this revision.Nov 9 2022, 3:38 AM

LGTM.

This revision is now accepted and ready to land.Nov 9 2022, 3:38 AM
foad added a comment.Dec 6 2022, 2:08 AM

Hi @kosarev, can this help with the following issue in FLAT_Load_Pseudo, or is it a different problem?

// FIXME: Operands with default values do not work with following non-optional operands.