This is an archive of the discontinued LLVM Phabricator instance.

[RISCV 6/10] Add basic RISCVAsmParser
ClosedPublic

Authored by asb on Aug 16 2016, 8:32 AM.

Details

Summary

This doesn't yet support parsing things like %pcrel_hi(foo), but will handle basic instructions with register or immediate operands.

Note: this patch is reliant on D23496

Diff Detail

Event Timeline

asb updated this revision to Diff 68189.Aug 16 2016, 8:32 AM
asb retitled this revision from to [RISCV 6/10] Add basic RISCVAsmParser.
asb updated this object.
asb added reviewers: theraven, jyknight.
asb added a subscriber: llvm-commits.
emaste added a subscriber: emaste.Aug 16 2016, 2:43 PM
t.p.northover added inline comments.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
299

I think this should be MatchOperand_NoMatch. ParseFail normally means you've already lexed something so backtracking is impossible.

reames added a subscriber: reames.Aug 21 2016, 12:25 PM
reames added inline comments.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
176

style: shouldn't this be createToken?

204

This seems odd. Is this a standard convention? Or can we lift this check into the caller and assert the Expr is not null?

347

This is a reasonable amount of parsing code without any associated tests. Can we write even trivial tests for this code at this point?

asb updated this revision to Diff 69352.Aug 26 2016, 5:24 AM
asb marked 4 inline comments as done.

Address review comments. Note that the way we defined AsmOperandClass has changed, which will be built upon in the next patch.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
204

Some targets define a whole bunch of AsmOperandClass, which means they'll have a lot of addFooOperands which eventually call addExpr. Checking !Expr in addExpr seems to be what the other backends I've checked do. That said, none of my testcases result in a null Expr so it may be one of those things that has been copied blindly by every backend author. I've added an assert to addExpr.

347

The next patch (D23564) adds tests and not much else (a very trivial instruction printer).

Just spotted one nit on my last pass through the code:

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
361–362

Isn't the first check redundant here?

jyknight added inline comments.Oct 3 2016, 8:12 AM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
31–34

Why are these here? Looks like MCTargetAsmParser already has a Parser member and these accessors.

35

Similarly, already stored in MCTargetAsmParser, accessible via getSTI().

asb updated this revision to Diff 74024.EditedOct 8 2016, 6:07 AM
asb marked 3 inline comments as done.

Address review comments from @jyknight and @t.p.northover. Also use # rather than !strconcat in RISCVInstrInfo.td

asb updated this revision to Diff 74225.Oct 11 2016, 3:52 AM

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors. Additionally, drop the unused getEndLoc after rL283691 removed it from MCParsedAsmOperand

jyknight accepted this revision.Oct 11 2016, 10:01 AM
jyknight edited edge metadata.
This revision is now accepted and ready to land.Oct 11 2016, 10:01 AM
niosHD added a subscriber: niosHD.Nov 22 2016, 12:29 AM
grosser added a subscriber: grosser.Jan 8 2017, 4:04 AM

Hi Alex,

I silently followed the RISC-V patches being upstreamed and did not see an update here for a little while. Is this patch blocked on something or did you just not get to this one yet?

Best,
Tobias

majnemer added inline comments.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
79–82

Isn't this just a StringRef?

asb added a comment.Feb 2 2017, 2:18 AM

Hi Alex,

I silently followed the RISC-V patches being upstreamed and did not see an update here for a little while. Is this patch blocked on something or did you just not get to this one yet?

Due to other responsibilities and my ongoing attempts to find further funding I'm a little behind where I wanted to be with the next set of patches. There were some concerns that aspects of this patchset might need adjusting once codegen was added, which is why I've held back on merging this part of the patchset. In retrospect, this probably wasn't necessary.

asb updated this revision to Diff 88305.Feb 13 2017, 9:55 PM
asb marked an inline comment as done.

Update to address review comment (just use StringRef rather than introducing Token struct).

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
79–82

Yes, I've replaced it with a StringRef now - thanks.

Razer6 added a subscriber: Razer6.Feb 14 2017, 6:29 AM
This revision was automatically updated to reflect the committed changes.
asb added a comment.Aug 8 2017, 9:22 AM

@alekseyshl: I've just committed rL310375 which should fix it.

asb added a comment.Aug 8 2017, 9:57 AM

RISCV should never have been included in LLVM_ALL_TARGETS when first committed - that was a mistake in the initial 'stub backend' patch. Removing RISCV from LLVM_ALL_TARGETS is the correct thing to do and shouldn't affect end-users, but I'll write to llvm-dev about this tomorrow for wider feedback.