%hi($tmp) and %lo($tmp) relocations for Mips backend, where $tmp = sym1 - sym2

[Mark Seaborn]

On 13 December 2013 07:23, Sasa Stankovic <Sasa.Stankovic at rt-rk.com> wrote:

> Hi Mark,
>
> The modified patch is attached. I used the method
> MCExpr::EvaluateAsAbsolute for computing the symbol value. This also
> handles the case of two symbols in different sections (EvaluateAsAbsolute
> won't compute symbol value in that case).
>

+$L1:
> +        .set    $temp1, $L1-$L2
> +        lui     $4, %hi($temp1)
> +        addiu   $4, $4, %lo($temp1)
> +$L2:
> +        .set    $temp2, $L2-$L1
> +        lui     $5, %hi($temp2)
> +        lw      $5, %lo($temp2)($5)
> +
> +# CHECK-INSTR:    lui     $4, 0
> +# CHECK-INSTR:    addiu   $4, $4, -8
> +# CHECK-INSTR:    lui     $5, 0
> +# CHECK-INSTR:    lw      $5, 8($5)


This test isn't testing that %hi works because your uses of %hi(X) always
evaluate to 0 in this test, which might easily happen by accident.  How
about changing it to do something like the following?

sym1:
.fill 0x43210
sym2:

Then you can test that %hi(sym2 - sym1) evaluates to 4.

@@ -297,6 +299,29 @@
>      // access to MCContext from here which allows us to report a fatal
> error
>      // with *possibly* a source code location.
>      (void)adjustFixupValue(Fixup, Value, &Asm.getContext());
>
+
> +    // Check for expressions %hi($tmp) and %lo($tmp), where $tmp is symbol
> +    // defined as a difference of two symbols, $tmp=sym1-sym2.
> +    if ((unsigned)Fixup.getKind() == Mips::fixup_Mips_HI16
> +        || (unsigned)Fixup.getKind() == Mips::fixup_Mips_LO16) {
>

Since the code below isn't specific to HI16 or LO16, could you remove this
check?  Maybe then the call to adjustFixupValue() should come after the new
code so that the value is checked for the other fixup types that have
constraints.

With that check removed, this is no longer MIPS-specific, so should the new
code go into evaluateFixup() in lib/MC/MCAssembler.cpp instead?  I tried
that and all the tests in test/MC/ still passed.


> +
> +      if (Target.getSymA() != NULL &&
> Target.getSymA()->getSymbol().isVariable()
> +          && Target.getSymB() == NULL && Target.getConstant() == 0) {
> +        const MCExpr *SymValue = Target.getSymA()->getSymbol()
> +                                       .getVariableValue();
> +        const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(SymValue);
> +
> +        if (BE && BE->getOpcode() == MCBinaryExpr::Sub) {
>

Since the code below isn't specific to MCBinaryExpr::Sub, I think you can
remove this check and just do SymValue->EvaluateAsAbsolute(AbsValue,
Layout).

Cheers,
Mark


> +          int64_t AbsValue;
> +          if (BE->EvaluateAsAbsolute(AbsValue, Layout)) {
> +            Value = AbsValue;
> +            // Don't emit relocation for expressions %hi(tmp) and
> %lo(tmp),
> +            // where tmp = sym1-sym2.
> +            IsResolved = true;
> +          }
> +        }
> +      }
> +    }
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131213/9505f705/attachment.html>