Patch implements %hi(sym1 - sym2) and %lo(sym1 - sym2) expressions for MIPS by creating target expression class MipsMCExpr.
Details
Diff Detail
Event Timeline
Thanks for uploading this via Phabricator. Sorry for being slow to review this again this time around -- I wanted to take the time to try the patch locally and try modifying it.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
12 | Nit: make sure the #include list stays sorted | |
1333 | So then, for clarity, these two lines: const MCExpr *LExp = evaluateRelocExpr(BE->getLHS(), RelocStr); const MCExpr *RExp = evaluateRelocExpr(BE->getRHS(), RelocStr); could be moved down to just before this line. | |
1334 | In your previous e-mail, you wrote:
I see what you mean. However, what I mean is that evaluateRelocExpr() shouldn't recurse when creating a MipsMCExpr. So it would be cleaner to change these two lines to: const MCSymbolRefExpr *LSRE = dyn_cast<MCSymbolRefExpr>(BE->getLHS()); const MCSymbolRefExpr *RSRE = dyn_cast<MCSymbolRefExpr>(BE->getRHS()); With that, the tests still pass. | |
1346 | With the change above, you don't need to recreate the two MCSymbolRefExprs here. You can just pass LSRE and RSRE to MCBinaryExpr::Create(). | |
1360–1361 | This 'default' case can be removed now, after adding the MCExpr::Target case, otherwise it produces the warning "default label in switch which covers all enumeration values [-Wcovered-switch-default]". | |
lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp | ||
397 | Note that this needs to be updated to isMicroMips(STI) on trunk now. | |
lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp | ||
36 | This check shouldn't be needed because this function can handle any subexpression. | |
lib/Target/Mips/MCTargetDesc/MipsMCExpr.h | ||
32 | Nit: you don't need a "_" prefix. You can write "Kind(Kind)" below. |
Thanks for addressing my comments. Just one more thing, about EvaluateAsRelocatableImpl(). I didn't understand that part when I reviewed the change on Wednesday, so I didn't comment on that part then, but having investigated I understand it now.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
1318 | Nit: With the change below, you could simplify this a little by using isa<> rather than dyn_cast<>. | |
1324 | Minor issue: You can simplify this further: you don't need to recreate the same MCBinaryExpr. CreateHi() and CreateLo() can be passed Expr rather than Res. | |
lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp | ||
38 | In your earlier version of the patch, you had EvaluateAsRelocatableImpl() doing the shifting/masking for hi16. I didn't understand how it worked to remove this part, until I investigated further. I had a look at the other *MCExpr.{h,cpp} files, and it seems there are there are two approaches to this:
This approach would work for MIPS if you:
(This works -- I tried it.)
For two architectures, EvaluateAsRelocatableImpl() just does "return false": For another two, EvaluateAsRelocatableImpl() delegates trivially to the subexpression: AArch64MCExpr::EvaluateAsRelocatableImpl(MCValue &Res, const MCAsmLayout *Layout) const { return getSubExpr()->EvaluateAsRelocatable(Res, *Layout); } Using approach 2 seems preferable since it's the most common in the codebase. It's what you're doing, except that your EvaluateAsRelocatableImpl() is a lot more complex and is closer to the PPC version. Based on the AArch64/Sparc versions, you can simplify this to: MipsMCExpr::EvaluateAsRelocatableImpl(MCValue &Res, const MCAsmLayout *Layout) const { if (!Layout) return false; return getSubExpr()->EvaluateAsRelocatable(Res, *Layout); } So can you change it to that, please? (The extra Layout check is necessary because getExprOpValue()'s call to EvaluateAsAbsolute() in MipsMCCodeEmitter.cpp causes this to be called with Layout==NULL.) |
LGTM. When you commit this, you can put "Differential Revision: http://llvm-reviews.chandlerc.com/D2592" in the commit message to link back to the review.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
1325 | Nit: you could do "return MipsMCExpr::CreateHi(...)" here | |
test/MC/Mips/hilo-addressing.s | ||
7 | Can you make this llvm-mc invocation consistent with the previous one? i.e. "%s" at end, and consistently use or omit "-o -". |
Nit: make sure the #include list stays sorted