This is an archive of the discontinued LLVM Phabricator instance.

[ms] [llvm-ml] Add support for attempted register parsing
ClosedPublic

Authored by epastor on Jan 27 2020, 8:08 AM.

Details

Summary

Add a new method (tryParseRegister) that attempts to parse a register specification.

MASM allows the use of IFDEF <register>, as well as IFDEF <symbol>. To accommodate this, we make it possible to check whether a register specification can be parsed at the current location, without failing the entire parse if it can't.

Diff Detail

Event Timeline

epastor created this revision.Jan 27 2020, 8:08 AM
epastor updated this revision to Diff 240597.Jan 27 2020, 8:10 AM

Split out tryParseRegister addition as a separate change, for easier review.

epastor updated this revision to Diff 240600.Jan 27 2020, 8:12 AM

Revert update from an incorrect commit

thakis added inline comments.Jan 27 2020, 8:31 AM
llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h
380 ↗(On Diff #240600)

Can you add a comment about what this does and what it's used for? (Just a summary of the cl desc)

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2340 ↗(On Diff #240600)

This not having an Error() suggests that maybe updateGprCountSymbols() emits it?

If it doesn't: Can this function now be implemented in terms of tryParseRegister()? (i.e. call that, and if it returns nullptr, emit Error(Tok.getLoc(), "not a valid operand")?)

llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
406 ↗(On Diff #240600)

This looks 100% identical to ParseRegister() in this file; can you implement Parse() in terms of tryParse() here too?

Also, isn't the return type here wrong? I'm guessing you didn't build with experimental targets enabled? (In the GN build, llvm_targets_to_build = "all" in your out\gn\args.gn enables all targets; in the cmake build it's -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR (iirc AVR is the only in-tree experimental target))

613 ↗(On Diff #240600)

Guess what my comment is here :)

llvm/lib/Target/BPF/AsmParser/BPFAsmParser.cpp
328 ↗(On Diff #240600)

Just like this! :)

llvm/lib/Target/Hexagon/AsmParser/HexagonAsmParser.cpp
1036 ↗(On Diff #240600)

I think it'd be good to be consistent in what function is implemented in terms of which one (i.e. implement Parse in terms of tryParse here too)

llvm/lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
708 ↗(On Diff #240600)

same comment here

llvm/lib/Target/MSP430/AsmParser/MSP430AsmParser.cpp
310 ↗(On Diff #240600)

well, you get the idea

Unit tests: pass. 62217 tests passed, 0 failed and 815 were skipped.

clang-tidy: fail. clang-tidy found 1 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: unknown.

clang-tidy: fail. clang-tidy found 4 errors and 69 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: pass. 62217 tests passed, 0 failed and 815 were skipped.

clang-tidy: fail. clang-tidy found 1 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

epastor updated this revision to Diff 240622.Jan 27 2020, 9:30 AM
epastor marked 13 inline comments as done.

Addressing comments.

Thanks for the feedback!

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2340 ↗(On Diff #240600)

updateGprCountSymbols does emit Errors, so I think we can't avoid that,

llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
406 ↗(On Diff #240600)

Not identical - note the UnLex lines 399-400. The key difference is to make sure that if the parse fails, we unroll everything we've done.

Also, the return type here is correct... I'd missed adding the definition above, instead. (And yes, didn't build with experimental targets enabled.)

613 ↗(On Diff #240600)

Nearly identical, but not quite - again, one unrolls what we lexed, and the other doesn't.

llvm/lib/Target/BPF/AsmParser/BPFAsmParser.cpp
328 ↗(On Diff #240600)

Doable here because ParseRegister doesn't lex any tokens if it fails.

llvm/lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
708 ↗(On Diff #240600)

Again, slightly different behavior - tryParse unlexes any tokens eaten, Parse doesn't.

Unit tests: fail. 62214 tests passed, 3 failed and 815 were skipped.

failed: LLVM.MC/MSP430/addrmode.s
failed: LLVM.MC/MSP430/reloc.s
failed: lld.ELF/msp430.s

clang-tidy: fail. clang-tidy found 1 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

epastor updated this revision to Diff 240645.Jan 27 2020, 11:19 AM

Fix MSP430 error.

Unit tests: fail. 62207 tests passed, 10 failed and 815 were skipped.

failed: Clang.CodeGen/msp430-fp-elim.c
failed: LLVM.CodeGen/MSP430/inline-asm-absolute-addressing.ll
failed: LLVM.CodeGen/MSP430/inline-asm.ll
failed: LLVM.MC/MSP430/addrmode.s
failed: LLVM.MC/MSP430/altreg.s
failed: LLVM.MC/MSP430/const.s
failed: LLVM.MC/MSP430/invalid.s
failed: LLVM.MC/MSP430/msp430-separator.s
failed: LLVM.MC/MSP430/opcode.s
failed: LLVM.MC/MSP430/reloc.s

clang-tidy: fail. clang-tidy found 1 errors and 2 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

lenary removed a subscriber: lenary.Jan 27 2020, 11:50 AM
epastor updated this revision to Diff 240652.Jan 27 2020, 11:53 AM

Actually fix MSP430 case (remove artifact of bad local changes)

thakis added inline comments.Jan 27 2020, 3:44 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2363 ↗(On Diff #240652)

But doesn't this then emit an error if we hit this code path?

llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
406 ↗(On Diff #240600)

Can we then maybe have both call a helper that takes a "Unlex" bool or something? Or have tryParse() return the token the error would have been emitted and then make Parse() set the current token to that in case of error? (The longer the implementation is, the more important this is – ie the X86 code below. But I'd try to be consistent in all parsers if possible.)

bcain added a subscriber: bcain.Jan 27 2020, 3:58 PM

HexagonAsmParser changes LGTM

nhaehnle removed a subscriber: nhaehnle.Jan 29 2020, 1:11 AM
epastor updated this revision to Diff 243256.Feb 7 2020, 11:42 AM

Merge logic to handle restoration after error more carefully, and with less duplication

epastor updated this revision to Diff 243263.Feb 7 2020, 12:19 PM

Finish merging logic between tryParseRegister and ParseRegister for remaining targets

epastor updated this revision to Diff 243285.Feb 7 2020, 2:11 PM

Fix formatting

epastor marked 4 inline comments as done.Feb 7 2020, 2:56 PM
epastor added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2363 ↗(On Diff #240652)

... Yep. Fixed, and code paths consolidated.

llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
406 ↗(On Diff #240600)

Done!

thakis added inline comments.Feb 10 2020, 7:18 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
2303 ↗(On Diff #243285)

Looks like this unlexes them left-to-right. Don't you have to unlex right-to-left?

llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
381 ↗(On Diff #243285)

Same question here. Isn't this in the wrong ordre?

llvm/lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
707 ↗(On Diff #243285)

Only if RestoreOnFailure (?)

714 ↗(On Diff #243285)

same

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
1159 ↗(On Diff #243285)

wrong direction again?

1192 ↗(On Diff #243285)

itto

1219 ↗(On Diff #243285)

ditto

1236 ↗(On Diff #243285)

ditto

(also, this happens often enough that you might want to put it in a helper lambda?)

1248 ↗(On Diff #243285)

ditto

1298 ↗(On Diff #243285)

ditto

epastor updated this revision to Diff 243597.Feb 10 2020, 9:26 AM
epastor marked 2 inline comments as done.

Fix UnLex ordering errors in tryParseRegister token restoration passes.

epastor updated this revision to Diff 243598.Feb 10 2020, 9:34 AM
epastor marked an inline comment as done.

Fix missed check in LanaiAsmParser (UnLex conditional on RestoreOnFailure)

epastor marked 8 inline comments as done.Feb 10 2020, 9:34 AM
epastor added inline comments.
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
1236 ↗(On Diff #243285)

Helper lambda added

epastor updated this revision to Diff 243621.Feb 10 2020, 10:23 AM
epastor marked an inline comment as done.

Rebase

thakis accepted this revision.Feb 10 2020, 1:15 PM

lgtm

Please add tests for the error cases when llvm-ml is hooked up to this.

Come to think of it, maybe just putting`assert(false && "not implemented") in the tryParse()` functions for non-x86 is good enough, since we don't currently expect to call them. But up to you.

This revision is now accepted and ready to land.Feb 10 2020, 1:15 PM
This revision was automatically updated to reflect the committed changes.