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

Sasa Stankovic Sasa.Stankovic at rt-rk.com
Tue Dec 24 12:39:42 PST 2013


Hi Mark,

> 1) Would it make sense to move this new logic into
> EvaluateAsRelocatable(),
> so that if EvaluateAsRelocatable() is called on a symbol S where S = sym2
> -
> sym1, then it returns an MCValue containing just a constant rather than
> returning MCValue::get(sym2, sym1, 0)?  Then the new logic would be
> applied
> in more cases -- it would apply to other callers of
> EvaluateAsRelocatable().
>
> Actually, it appears the way to do that would just be to remove the
> "SRE->getKind() == MCSymbolRefExpr::VK_None" check from
> MCExpr::EvaluateAsRelocatableImpl()'s handling of SymbolRef in MCExpr.cpp.
> Isn't your patch roughly equivalent to that?  Are there cases where that
> check is necessary for correctness?  Removing it doesn't seem to cause any
> tests to fail.

I don't know whether any target supports nested relocation expressions (or
whether it is correct at all to create such expressions), but if it does,
then the check SRE->getKind() == MCSymbolRefExpr::VK_None is necessary to
prevent wrong calculation of expression value. Here is a simple example:

$L1:
.set    $temp1, $L2-$L1
.set    $temp2, %hi($temp1)
addiu   $4, $4, %lo($temp2)
$L2:

Mips LLVM assembler (or gas) won't parse directive ".set temp2, %hi(temp1)",
but this example can be easily created in code:

MCContext *Ctx;
MCSymbol *Temp1 = Ctx->CreateTempSymbol();
Temp1->setVariableValue(MCBinaryExpr::CreateSub(L2, L1, *Ctx));

MCSymbol *Temp2 = Ctx->CreateTempSymbol();
Temp2->setVariableValue(MCSymbolRefExpr::Create(Temp1,
                          MCSymbolRefExpr::VK_Mips_ABS_HI, *Ctx));

const MCSymbolRefExpr *Lo = MCSymbolRefExpr::Create(Temp2,
                              MCSymbolRefExpr::VK_Mips_ABS_LO, *Ctx);
ADDIU.addOperand(MCOperand::CreateExpr(Lo));

If check SRE->getKind() == MCSymbolRefExpr::VK_None is removed from
MCExpr::EvaluateAsRelocatableImpl(), %lo($temp2) will evaluate to 4 (and
no relocation will be emitted). This is obviously wrong since %lo($temp2)
= %lo(%hi($L2-$L1)) = 0. So, by removing the check, the inner %hi
relocation was ignored and method only evaluated %lo($L2-$L1).

> 2) Why is it that you handle cases like:
>
> .set diff, label2 - label1
> lui $2, %lo(diff)
>
> but not cases where that is written more directly as:
>
> lui $2, %lo(label2 - label1)
>
> (which still gives the UNREACHABLE error I described earlier in the
> thread)?  Is it just because the former is easier to implement?  If so, is
> that just a quirk of how these cases are handled via different code paths
> in MC?

Implementing %lo(label2 - label1) was first thing that I tried. The
problem was that no MCBinaryExpr constructor takes relocation kind as
argument; relocations can only be passed to MCSymbolRefExpr. Currently,
when MipsAsmParser parses %lo(label2 - label1), it creates MCBinaryExpr
where both operands are MCSymbolRefExpr, and both operands have relocation
kind for %lo relocation (VK_Mips_ABS_LO). This is the reason for
UNREACHABLE error, because there are two identical relocations for the
same offset in .o file.

I then tried to pass VK_Mips_None instead of VK_Mips_ABS_LO to second
operand, but this also produced couple of UNREACHABLE errors, this time in
Mips specific code.

Finally, I created target-specific MCExpr subclass, as a subclass of
MCTargetExpr. The implementation was similar to
lib/Target/ARM/MCTargetDesc/ARMMCExpr.h. This class has members for
expression and relocation kind, so associating relocation to expression as
a whole was easy.

The patch worked, but then people at Mips noted that gas supports
assigning symbol difference to temp symbol and applying %hi and %lo on
that symbol. That seemed much simpler.

Regards,
Sasa






More information about the llvm-commits mailing list