This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Fix some ambiguous cases in AsmParser
ClosedPublic

Authored by benshi001 on Dec 30 2022, 4:14 AM.

Details

Summary

Some specific operands in specific instructions should be treated
as variables/symbols/labels, other than registers.

This patch fixes those ambiguous cases, such as "lds r25, r24",
which means loading the value inside symbol 'r24' into register 'r25'.

Diff Detail

Event Timeline

benshi001 created this revision.Dec 30 2022, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2022, 4:14 AM
Herald added subscribers: Jim, hiraditya. · View Herald Transcript
benshi001 requested review of this revision.Dec 30 2022, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2022, 4:14 AM
benshi001 updated this revision to Diff 485686.Dec 30 2022, 4:20 AM
benshi001 retitled this revision from [AVR] Fix ambiguous cases in AsmParser to [AVR] Fix some ambiguous cases in AsmParser.
benshi001 edited the summary of this revision. (Show Details)
benshi001 updated this revision to Diff 485697.Dec 30 2022, 7:18 AM
benshi001 edited the summary of this revision. (Show Details)

All the tests use register names as symbols/labels/expressions, such as "jmp x + 2", "lds reg, r1", "adiw reg, y + 2".

aykevl added inline comments.Jan 1 2023, 11:00 AM
llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
630–632

I think the following is a lot easier to read:

OperandNum++;
if (OperandNum > 1)
  eatComma();

If you want OperandNum to start at 0, I suggest initializing it to -1:

int OperandNum = -1;
651

std::array is probably a better choice in this case. See: https://reviews.llvm.org/D140569#inline-1358484

Also: can you add a test for adiw and sbiw?

652–656

Style issue: use braces for the outer if since the nested for is braced. See: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

for (auto Inst : Insts) {
  if (Inst == Mnemonic) {
    maybeReg = false;
    break;
  }
}

Same for the loop below.

benshi001 updated this revision to Diff 485833.Jan 1 2023, 5:53 PM
benshi001 marked 3 inline comments as done.
benshi001 added inline comments.
llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
651

Done. Changed to std::array, and also tests for adiw/sbiw are added.

benshi001 marked an inline comment as done.Jan 1 2023, 5:55 PM
benshi001 added inline comments.
llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
630–632

Thanks. Using int OperandNum = -1; looks more clear.

benshi001 added inline comments.Jan 1 2023, 5:57 PM
llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
659

there may be other instructions which need a careful check. So I temporarily give the array a length=8.

aykevl accepted this revision.Jan 6 2023, 1:16 PM
This revision is now accepted and ready to land.Jan 6 2023, 1:16 PM
This revision was landed with ongoing or failed builds.Jan 6 2023, 6:42 PM
This revision was automatically updated to reflect the committed changes.