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

[Mark Seaborn]

  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.


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp:377
@@ +376,3 @@
+    case MipsMCExpr::VK_Mips_ABS_HI:
+      FixupKind = MCFixupKind(IsMicroMips
+                              ? Mips::fixup_MICROMIPS_HI16
----------------
Note that this needs to be updated to isMicroMips(STI) on trunk now.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1374
@@ -1345,2 +1373,3 @@
+    return true;
   default:
     return false;
----------------
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]".

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:11
@@ -10,2 +10,3 @@
 #include "MCTargetDesc/MipsMCTargetDesc.h"
+#include "MCTargetDesc/MipsMCExpr.h"
 #include "MipsRegisterInfo.h"
----------------
Nit: make sure the #include list stays sorted

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1319
@@ +1318,3 @@
+
+    const MCSymbolRefExpr *LSRE = dyn_cast<MCSymbolRefExpr>(LExp);
+    const MCSymbolRefExpr *RSRE = dyn_cast<MCSymbolRefExpr>(RExp);
----------------
In your previous e-mail, you wrote:
> If evaluateRelocExpr() is removed, then following instruction won't assemble: ...

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.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1344
@@ -1317,2 +1343,3 @@
+
     Res = MCBinaryExpr::Create(BE->getOpcode(), LExp, RExp, getContext());
     return Res;
----------------
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.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp:35
@@ +34,3 @@
+  assert(SRE1 && SRE2
+         && "Only sym1-sym2 target expressions are currently supported.");
+
----------------
This check shouldn't be needed because this function can handle any subexpression.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1331
@@ +1330,3 @@
+      // needed for MipsMCExpr::EvaluateAsRelocatableImpl to work correctly.
+      const MCSymbolRefExpr *LSRE2
+        = MCSymbolRefExpr::Create(&LSRE->getSymbol(), getContext());
----------------
With the change above, you don't need to recreate the two MCSymbolRefExprs here.  You can just pass LSRE and RSRE to MCBinaryExpr::Create().

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.h:31
@@ +30,3 @@
+
+  explicit MipsMCExpr(VariantKind _Kind, const MCExpr *_Expr)
+    : Kind(_Kind), Expr(_Expr) {}
----------------
Nit: you don't need a "_" prefix.  You can write "Kind(Kind)" below.


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