[PATCH] Allowing MC backends to decide relaxation based on fixup resolution

[Jim Grosbach]

> On Mar 25, 2015, at 9:14 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> If the issue is just having to change every backend, you could use a
> solution with two virtual functions:
> 
> virtual bool simpleFixupNeedsRelaxation(const MCFixup &Fixup,
>                                    uint64_t Value,
>                                    const MCRelaxableFragment *DF,
>                                    const MCAsmLayout &Layout) const = 0;
> 
> virtual bool fixupNeedsRelaxation(const MCFixup &Fixup, bool Resolved,
>                                    uint64_t Value,
>                                    const MCRelaxableFragment *DF,
>                                    const MCAsmLayout &Layout) const {
> if(!Resolved)
>    return true;
> 
>  return simpleFixupNeedsRelaxation(Fixup, Value, DF, Layout);
> }
> 
> That way Hexagon overrides fixupNeedsRelaxation and other targets
> override simpleFixupNeedsRelaxation.
> 

Hrm… That could work. Still feels a bit hacky, but much better than scattering bits of logic into all of the backends.

> 
> On 25 March 2015 at 12:04, Colin LeMahieu <colinl at codeaurora.org> wrote:
>> In http://reviews.llvm.org/D8217#146472, @grosbach wrote:
>> 
>>> In http://reviews.llvm.org/D8217#144272, @colinl wrote:
>>> 
>>>> I'm definitely open to cleaner implementations.
>>>> 
>>>> The issue with not doing anything in relaxInstruction() is relaxation will get stuck in an infinite loop.  The relaxation loop assumes that relaxInstruction will eventually change fixupNeedsRelaxation or mayNeedRelaxation to false.
>>> 
>>> 
>>> Yeah, we'll likely need to do something there. My inclination is do something with the target-specific fixups. Perhaps use a different fixup kind which isn't relaxable?
>> 
>> 
>> That's a possibility I hadn't considered before.  One concern is we wouldn't want to lose the target specific MCFixupKind so it seems like it would have to be a flag instead of a new enumeration.
>> 
>> One method I thought of instead of using a bool flag would simply be to pass is a maximum for Value when calling Backend::fixupNeedsRelaxation.  Conceptually this is what we're saying anyway, we don't know where this value is so it could be anywhere up to the maximum value away.
>> If we were to pass in some maximum value we'd have to consider if it's going to me max int64_t or max uint64_t.  During evaluateFixup it looks like the first place Value is assigned is from an int64_t and later some unsigned offsets get added but the whole value is converted to an uint64_t but what we're really expressing is a signed displacement.

A magic value could work. It feels a bit dubious, though, as there’s no theoretical reason a fixup couldn’t have a legal value that large.

If we’re adding a new flag, it should probably be on the MCRelaxableFragment fragment rather than the fixup itself. That way where the flag is kept has the same scope/lifetime/etc as the algorithm that’s using it. What do you think, Colin?

>> Another way I didn't care for as much was having Backend::relaxInstruction return whether it relaxed the instruction.  This would still have the drawbacks of requiring all backends to change their interface and it's weird that fixupNeedsRelaxation would return true but relaxInstruction would say something different.
>> 

Yep. That’s the same core issue, just in a different interface.

>> 
>> REPOSITORY
>>  rL LLVM
>> 
>> http://reviews.llvm.org/D8217
>> 
>> EMAIL PREFERENCES
>>  http://reviews.llvm.org/settings/panel/emailpreferences/
>> 
>>