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

Sasa Stankovic Sasa.Stankovic at rt-rk.com
Wed Jan 22 09:10:10 PST 2014


Hi Mark,

> Can you use Phabricator (http://llvm.org/docs/Phabricator.html) to send
> patches in future?  It will make reviewing easier.

I created Phabricator account and uploaded the patch.  I'm not sure
whether I should reply in Phabricator's summary field or by mail, so for
now I'm using the mail.

> This evaluateRelocExpr() function is what is doing the incorrect
> conversion
> from %hi(X - Y) to (%hi(X) - %hi(Y)), so really those two recursive calls
> to evaluateRelocExpr() above should go away.  I don't think there's any
> reason for evaluateRelocExpr() to recurse on an MCBinaryExpr, is there?

If evaluateRelocExpr() is removed, then following instruction won't assemble:

$L1:
    addiu   $4, $4, %lo($L1 + 4)

>> @@ -365,6 +366,28 @@
>>      Res += getExprOpValue(cast<MCBinaryExpr>(Expr)->getRHS(), Fixups);
>>      return Res;
>>    }
>> +
>> +  if (Kind == MCExpr::Target) {
>> +    const MipsMCExpr *MipsExpr = cast<MipsMCExpr>(Expr);
>> +
>> +    MCFixupKind FixupKind;
>> +    switch (MipsExpr->getKind()) {
>> +    default: llvm_unreachable("Unsupported fixup");
>> +    case MipsMCExpr::VK_Mips_ABS_HI:
>> +      FixupKind = MCFixupKind(IsMicroMips
>> +                              ? Mips::fixup_MICROMIPS_HI16
>> +                              : Mips::fixup_Mips_HI16);
>> +      break;
>> +    case MipsMCExpr::VK_Mips_ABS_LO:
>> +      FixupKind = MCFixupKind(IsMicroMips
>> +                              ? Mips::fixup_MICROMIPS_LO16
>> +                              : Mips::fixup_Mips_LO16);
>> +      break;
>> +    }
>> +    Fixups.push_back(MCFixup::Create(0, MipsExpr, FixupKind));
>> +    return 0;
>> +  }
>> +
>>
>
> I'm not sure what case this covers, but if I remove it the tests pass, so
> maybe either leave it out or add a test?

The test that is part of the patch won't pass without it.

>> +bool
>> +MipsMCExpr::EvaluateAsRelocatableImpl(MCValue &Res,
>> +                                      const MCAsmLayout *Layout) const
>> {
>> +  MCValue Value;
>> +
>> +  if (!Layout || !getSubExpr()->EvaluateAsRelocatable(Value, *Layout))
>> +    return false;
>> +
>> +  if (Value.isAbsolute()) {
>> +    int64_t Result = Value.getConstant();
>> +    switch (Kind) {
>> +      default:
>> +        llvm_unreachable("Invalid kind!");
>> +      case VK_Mips_ABS_LO:
>> +        Result = Result & 0xffff;
>> +        break;
>> +      case VK_Mips_ABS_HI:
>> +        Result = ((Result + 0x8000) >> 16) & 0xffff;
>> +        break;
>> +    }
>> +    Res = MCValue::get(Result);
>> +  } else {
>> +    MCContext &Context = Layout->getAssembler().getContext();
>>
>
> It looks like you don't need the code for this else branch since you never
> create an MipsMCExpr wrapping an MCSymbolRefExpr, so this can just return
> false.

The else branch is executed if the expression is relocatable but not
absolute.  It has to create relocatable expression in variable Res and
return true.

>> @@ -129,8 +130,10 @@
>>      const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(BE->getRHS());
>>      assert(SRE && CE && "Binary expression must be sym+const.");
>>      Offset = CE->getValue();
>> -  }
>> -  else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))
>> +  } else if (const MipsMCExpr *ME = dyn_cast<MipsMCExpr>(Expr)) {
>> +    ME->PrintImpl(OS);
>> +    return;
>> +  } else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))
>>      assert(false && "Unexpected MCExpr type.");
>>
>
> Why not just add a general-purpose "else" which calls ME->print(OS)?

Last "else if" just checks that the only remaining case that is allowed is
MCSymbolRefExpr.  The code that actually prints MCSymbolRefExpr is below
it and is more complex than just calling Expr->print(OS) (in fact, it
handles MCBinaryExpr too).


Regards,
Sasa







More information about the llvm-commits mailing list