Page MenuHomePhabricator

[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

Repository
rL LLVM

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
298 ↗(On Diff #68189)

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
175 ↗(On Diff #68189)

style: shouldn't this be createToken?

203 ↗(On Diff #68189)

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

346 ↗(On Diff #68189)

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
203 ↗(On Diff #68189)

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.

346 ↗(On Diff #68189)

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
360–361 ↗(On Diff #69352)

Isn't the first check redundant here?

jyknight added inline comments.Oct 3 2016, 8:12 AM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
30–33 ↗(On Diff #69352)

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

34 ↗(On Diff #69352)

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
78–81 ↗(On Diff #74225)

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
78–81 ↗(On Diff #74225)

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.