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

[Sasa Stankovic]

Hi Mark,

The patch is attached.

Regards,
Sasa


> 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 --------------
A non-text attachment was scrubbed...
Name: hilo_sym_diff3.patch
Type: text/x-patch
Size: 3108 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131216/4ae6cb37/attachment.bin>