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

[Toma Tabacu]

================
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;
 }
 
----------------
dsanders wrote:
> 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.
It's true that this function needs to be revised a bit, but I don't think it should be a blocker for this patch, as long as the output is semantically equivalent to GAS' (see my reply to the comment below).

================
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]
----------------
dsanders wrote:
> 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)
AFAICT the examples you give do not match GAS' output.
The expansions from the patch are closer to the ones from GAS, the only difference being that GAS uses ADDiu and we use ORi.

http://reviews.llvm.org/D9344

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