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

[Vladimir Stefanovic]

REPOSITORY
  rL LLVM

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:29
@@ +28,3 @@
+struct MipsRelocationEntry {
+  MipsRelocationEntry(ELFRelocationEntry &R, unsigned MatchingLo):
+      Reloc(R), SortOffset(R.Offset), MatchingLoType(MatchingLo),
----------------
rafael wrote:
> Please use the same names for arguments and fields so that you have
> ... : foo(foo), bar(bar) ... {
OK.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:33
@@ +32,3 @@
+
+  ELFRelocationEntry Reloc;
+  // SortOffset equals Reloc.Offset except for the *HI16 relocations, for which
----------------
rafael wrote:
> Can this be a
> 
> const  ELFRelocationEntry &Reloc;
> 
> ?
It can be const, but '&' would bring down the last step, which copies sorted MipsRelocs back to Relocs.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:39
@@ +38,3 @@
+  // For *HI16 relocations, a matching relocation type for Reloc.Type.
+  int64_t MatchingLoType;
+  // True when this is a *LO16 relocation preceded with a matching *HI16
----------------
rafael wrote:
> Why do you need to store this?
It isn't necessary, I can always call getMatchingLoType() instead, which I did in the updated patch.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:278
@@ +277,3 @@
+
+  // Don't check if symbol is local for non-*GOT16 relocations.
+  if (!(Reloc.Type == ELF::R_MIPS_GOT16
----------------
rafael wrote:
> Why is that?
Just to skip calling Asm.getSymbolData() for the relocations that will return R_MIPS_NONE anyway. I removed it.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:285
@@ +284,3 @@
+  const MCSymbolData &SD = Asm.getSymbolData(*Reloc.Symbol);
+  if (MCELF::GetBinding(SD) == ELF::STB_LOCAL) {
+    if (Reloc.Type == ELF::R_MIPS_GOT16) return ELF::R_MIPS_LO16;
----------------
rafael wrote:
> Use an earlier return
> 
>  if (MCELF::GetBinding(SD) != ELF::STB_LOCAL)
>     return ELF::R_MIPS_NONE;
> 
OK.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:309
@@ +308,3 @@
+  Hi.IsNextMatchingLo = true;
+  Hi.SortOffset = Lo.Reloc.Offset -1;
+}
----------------
rafael wrote:
> 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.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:335
@@ +334,3 @@
+// rule, we try to replicate binutils' behaviour, for no other reason than
+// consistency with what gnu as produces.
+//
----------------
rafael wrote:
> This wording is a bit soft.
> 
> 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.


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:344
@@ +343,3 @@
+// When there are more HIs matched with one LO, sort them in descending order by
+// offset.
+//
----------------
rafael wrote:
> This description makes it sound like that given
> 
> lui    $2, %hi(func2)
> lui    $2, %hi(func2)
> addiu  $2, $2, %lo(func2)
> addiu  $2, $2, %lo(func2)
> 
> the second %hi would be matched with the second %lo, but in a previous comment you said that gas produces
> 
> 
> 00000004 00000605 R_MIPS_HI16 00000000 func2
> 00000008 00000606 R_MIPS_LO16 00000000 func2
> 00000000 00000605 R_MIPS_HI16 00000000 func2
> 0000000c 00000606 R_MIPS_LO16 00000000 func2
> 
> That is, the second %hi (offset 4) is matched with the first lo (offset 8).
> 
That's right, because the second %hi is already followed by a %lo. So the sentence about finding the matching %lo applies only to the first %hi, which gets matched with the second %lo.
I tried to be clearer in the next paragraph, with "don't look for a 'better' match for the HIs that are already followed with a matching LO".


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:353
@@ +352,3 @@
+                                     std::vector<ELFRelocationEntry> &Relocs) {
+  if (Relocs.size() < 2) return;
+
----------------
rafael wrote:
> Please git-clang-format the patch.
Sure.

================
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.

================
Comment at: test/MC/Mips/sort-relocation-table.s:6
@@ +5,3 @@
+# corresponding *LO16 relocation against the same symbol.
+
+# TODO: Add mips16 and micromips tests.
----------------
rafael wrote:
> Can you add a comment about the gnu as target and command line that can be used to compare the result of this file?
Added.

http://reviews.llvm.org/D7414

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/