[PATCH] Patch that implements %hi(sym1 - sym2) and %lo(sym1 - sym2) expressions for MIPS.

[Mark Seaborn]

  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.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1324
@@ +1323,3 @@
+      // Create target expression for %hi(sym1-sym2) and %lo(sym1-sym2).
+      Res = MCBinaryExpr::Create(BE->getOpcode(), LSRE, RSRE, getContext());
+      if (VK == MCSymbolRefExpr::VK_Mips_ABS_HI)
----------------
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.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1318
@@ +1317,3 @@
+    // Check for %hi(sym1-sym2) and %lo(sym1-sym2) expressions.
+    const MCSymbolRefExpr *LSRE = dyn_cast<MCSymbolRefExpr>(BE->getLHS());
+    const MCSymbolRefExpr *RSRE = dyn_cast<MCSymbolRefExpr>(BE->getRHS());
----------------
Nit: With the change below, you could simplify this a little by using isa<> rather than dyn_cast<>.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp:37
@@ +36,3 @@
+bool
+MipsMCExpr::EvaluateAsRelocatableImpl(MCValue &Res,
+                                      const MCAsmLayout *Layout) const {
----------------
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:

1) Have EvaluateAsRelocatableImpl() do the shifting/masking.  There's only one architecture that does this, PowerPC (see lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.cpp).

This approach would work for MIPS if you:
 * change MipsMCCodeEmitter.cpp's MCExpr::Target handling to push a FK_Data_2 fixup instead of LO16/HI16 fixup;
 * change adjustFixupValue() in MipsAsmBackend.cpp to accept FK_Data_2 in addition to FK_Data_4.
(This works -- I tried it.)

2) Do the shifting/masking elsewhere, leaving a trivial implementation of EvaluateAsRelocatableImpl().

For two architectures, EvaluateAsRelocatableImpl() just does "return false":
lib/Target/ARM/MCTargetDesc/ARMMCExpr.cpp
lib/Target/NVPTX/NVPTXMCExpr.h

For another two, EvaluateAsRelocatableImpl() delegates trivially to the subexpression:
lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp
lib/Target/Sparc/MCTargetDesc/SparcMCExpr.cpp
i.e.
  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.)



http://llvm-reviews.chandlerc.com/D2592