[PATCH] Re-enable a hook in MCELFObjectTargetWriter to allow target-specific relocationtable sorting and use this hook for Mips.

Rafael EspĂ­ndola rafael.espindola at gmail.com
Wed Mar 18 10:07:16 PDT 2015


>> Why -1?
>> This looks like a leftover from the sort function not considering a relocation own offset to break ties.
> '-1' simplifies the sort function.
>
> If omitting '-1', the rules for entries with the same SortOffset would be:
> -if one is %hi, that one should be first;
> -if both are %hi, sort them by Offset (in descending order).
>
> So '-1' practically applies the first rule, and the sorting function only needs to handle the second case.
>

But it cant introduce a wrong result if another relocation is 1 byte
away, no? We just know that that is not possible? If so please add a
comment about that.

>> Assembly is user input, and if gnu as matches a %lo and a %hi differently from what we do we would be producing a .o with different semantics.
> How about:
>   // When there is more than one way to sort the table while respecting the ABI
>   // rule, we try to implement the same semantics as gnu as, ie. we try to match
>   // a HI and a LO the same way gnu as does.

Still not there. Which relocation gets matched to another is part of
of the semantics of the assembly file. We have to match gas or
document that we are incompatible.

> ================
> Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:366
> @@ +365,3 @@
> +    // Also find HIs and LOs that are already matched.
> +    if (I > 0 && areMatchingHiAndLo(MipsRelocs[I], MipsRelocs[I-1]))
> +      setMatch(MipsRelocs[I], MipsRelocs[I-1]);
> ----------------
> rafael wrote:
>> Why is this first check for I and I-1 necessary? The main loop should handle it, no?
> It finds 'non-free' %lo's, ie. the ones that are already preceded with a %hi. Then it's easier for the main loop to find a 'free' %lo for a %hi, which are preferred over the 'non-free' ones. As an addition, it remembers %hi's with a matched %lo so the main loop can early exit for them.

The rules around this area are pretty fuzzy. Lets try to make the code
as direct as possible on the first implementation at least. Could you
please remove this case and let the main loop handle it?

I will take another look in phab.




More information about the llvm-commits mailing list