[PATCH] [mips] [IAS] Fix expansion of LASym with positive offset.

[Daniel Sanders]

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2526-2527
@@ -2532,4 +2525,4 @@
 
 const MCExpr *MipsAsmParser::evaluateRelocExpr(const MCExpr *Expr,
                                                StringRef RelocStr) {
   const MCExpr *Res;
----------------
dsanders wrote:
> This code seems suspicious. For example, the MCBinaryExpr path tries to apply the RelocStr to both sides of the operator but it's easy to think of cases where this is incorrect. For example %hi(symbol+1) is not equivalent to %hi(symbol) + %hi(1) when symbol==0. The former results in %hi(1) = 0 while the latter results in %hi(0) + %hi(1) = 1.
Despite it's name, I don't think this is really evaluating expressions. It seems to be simplifying them.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2526-2585
@@ -2532,62 +2525,62 @@
 
 const MCExpr *MipsAsmParser::evaluateRelocExpr(const MCExpr *Expr,
                                                StringRef RelocStr) {
   const MCExpr *Res;
   // Check the type of the expression.
   if (const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(Expr)) {
     // It's a constant, evaluate reloc value.
     int16_t Val;
     switch (getVariantKind(RelocStr)) {
     case MCSymbolRefExpr::VK_Mips_ABS_LO:
       // Get the 1st 16-bits.
       Val = MCE->getValue() & 0xffff;
       break;
     case MCSymbolRefExpr::VK_Mips_ABS_HI:
       // Get the 2nd 16-bits. Also add 1 if bit 15 is 1, to compensate for low
       // 16 bits being negative.
       Val = ((MCE->getValue() + 0x8000) >> 16) & 0xffff;
       break;
     case MCSymbolRefExpr::VK_Mips_HIGHER:
       // Get the 3rd 16-bits.
       Val = ((MCE->getValue() + 0x80008000LL) >> 32) & 0xffff;
       break;
     case MCSymbolRefExpr::VK_Mips_HIGHEST:
       // Get the 4th 16-bits.
       Val = ((MCE->getValue() + 0x800080008000LL) >> 48) & 0xffff;
       break;
     default:
       report_fatal_error("unsupported reloc value");
     }
     return MCConstantExpr::Create(Val, getContext());
   }
 
   if (const MCSymbolRefExpr *MSRE = dyn_cast<MCSymbolRefExpr>(Expr)) {
     // It's a symbol, create a symbolic expression from the symbol.
     StringRef Symbol = MSRE->getSymbol().getName();
     MCSymbolRefExpr::VariantKind VK = getVariantKind(RelocStr);
     Res = MCSymbolRefExpr::Create(Symbol, VK, getContext());
     return Res;
   }
 
   if (const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr)) {
     MCSymbolRefExpr::VariantKind VK = getVariantKind(RelocStr);
 
     // Try to create target expression.
     if (MipsMCExpr::isSupportedBinaryExpr(VK, BE))
       return MipsMCExpr::Create(VK, Expr, getContext());
 
     const MCExpr *LExp = evaluateRelocExpr(BE->getLHS(), RelocStr);
     const MCExpr *RExp = evaluateRelocExpr(BE->getRHS(), RelocStr);
     Res = MCBinaryExpr::Create(BE->getOpcode(), LExp, RExp, getContext());
     return Res;
   }
 
   if (const MCUnaryExpr *UN = dyn_cast<MCUnaryExpr>(Expr)) {
     const MCExpr *UnExp = evaluateRelocExpr(UN->getSubExpr(), RelocStr);
     Res = MCUnaryExpr::Create(UN->getOpcode(), UnExp, getContext());
     return Res;
   }
   // Just return the original expression.
   return Expr;
 }
 
----------------
This code seems suspicious. For example, the MCBinaryExpr path tries to apply the RelocStr to both sides of the operator but it's easy to think of cases where this is incorrect. For example %hi(symbol+1) is not equivalent to %hi(symbol) + %hi(1) when symbol==0. The former results in %hi(1) = 0 while the latter results in %hi(0) + %hi(1) = 1.

================
Comment at: test/MC/Mips/mips-expansions.s:28-44
@@ -27,2 +27,19 @@
                                      #   fixup A - offset: 0, value: symbol at ABS_LO, kind: fixup_Mips_LO16
+# CHECK: lui $8, %hi(symbol)           # encoding: [A,A,0x08,0x3c]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_HI, kind: fixup_Mips_HI16
+# CHECK: ori $8, $8, %lo(symbol+1)     # encoding: [0x01'A',A,0x08,0x35]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_LO, kind: fixup_Mips_LO16
+# CHECK: lui $8, %hi(symbol+1)         # encoding: [0x01'A',A,0x08,0x3c]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_HI, kind: fixup_Mips_HI16
+# CHECK: ori $8, $8, %lo(symbol-32766) # encoding: [0x02'A',0x80'A',0x08,0x35]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_LO, kind: fixup_Mips_LO16
+# CHECK: lui $8, %hi(symbol+1)         # encoding: [0x01'A',A,0x08,0x3c]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_HI, kind: fixup_Mips_HI16
+# CHECK: ori $8, $8, %lo(symbol+2)     # encoding: [0x02'A',A,0x08,0x35]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_LO, kind: fixup_Mips_LO16
+# CHECK: lui $8, %hi(symbol+1)         # encoding: [0x01'A',A,0x08,0x3c]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_HI, kind: fixup_Mips_HI16
+# CHECK: ori $8, $8, %lo(symbol+4369)  # encoding: [0x11'A',0x11'A',0x08,0x35]
+# CHECK:                               #   fixup A - offset: 0, value: symbol at ABS_LO, kind: fixup_Mips_LO16
+
 # CHECK: lui     $10, %hi(symbol)        # encoding: [A,A,0x0a,0x3c]
----------------
These look wrong to me. I'd expect the same expression inside both the %hi and %lo's like so:
lui $8, %hi(symbol+1)
ori $8, $8, %lo(symbol+1)
lui $8, %hi(symbol+32770)
ori $8, $8, %lo(symbol+32770)
lui $8, %hi(symbol+65538)
ori $8, $8, %lo(symbol+65538)
lui $8, %hi(symbol+1118481)
ori $8, $8, %lo(symbol+1118481)

http://reviews.llvm.org/D9344

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/