[x86] Fix nasty bug in the x86 backend that is essentially impossible to hit from IR but creates a minefield for MI passes.

[chandlerc]

Summary

The x86 backend has fairly powerful logic to try and fold loads that
feed register operands to instructions into a memory operand on the
instruction. This is almost always a good thing, but there are specific
relocated loads that are only allowed to appear in specific
instructions. Notably, R_X86_64_GOTTPOFF is only allowed in movq and
addq. This patch blocks folding of memory operands using this
relocation unless the target is in fact addq.

The particular relocation indicates why we simply don't hit this under
normal circumstances. This relocation is only used for TLS, and it gets
used in very specific ways in conjunction with %fs-relative addressing.
The result is that loads using this relocation are essentially never
eligible for folding into an instruction's memory operands. Unless, of
course, you have an MI pass that inserts usage of such a load. I have
exactly such an MI pass and was greeted by truly mysterious miscompiles
where the linker replaced my instruction with a completely garbage byte
sequence. Go team.

This is the only such relocation I'm aware of in x86, but there may be
others that need to be similarly restricted.

Fixes PR36165.

Note, I have no real idea how I *should* be testing this or *should* be
implementing this. It is essentially a stab in the dark. Please feel free to
suggest more effective ways to test, implement, or otherwise go about this.

[probinson]

llvm/test/CodeGen/X86/bad-tls-fold.mir

19 ↗

(On Diff #132141)

Drive-by nit: You don't need the poorly named -LABEL suffix (it doesn't actually identify labels).

41 ↗

(On Diff #132141)

No need for -LABEL here.

[chandlerc]

llvm/test/CodeGen/X86/bad-tls-fold.mir

19 ↗

(On Diff #132141)

I don't understand...

My understanding of CHECK-LABEL is that it partitions the CHECK-reported errors so that its easier to understand them. I want that partition to hook on the label or rather than the orq instruction, or some other use of the word or, so it seems reasonable to include the suffix of :? In IR tests we routinely do CHECK-LABLE: @foo( to ensure we don't match a function by the name of @foo.bar.

41 ↗

(On Diff #132141)

As above, I'm not imagining that this is *necessary*, but it seems good hygiene to separate out the blocks of checks.

[chandlerc]

(Also, thanks for review on my first ever usage of MIR! I have so little idea of what I'm doing with it....)

[probinson]

llvm/test/CodeGen/X86/bad-tls-fold.mir

19 ↗

(On Diff #132141)

Oh, I didn't notice the other CHECK lines. Duh.
Please make all the CHECK lines use consistent indentation and comment characters to accommodate the eyesight-challenged. :-P

[chandlerc]

llvm/test/CodeGen/X86/bad-tls-fold.mir

19 ↗

(On Diff #132141)

I would actually love to do this. However, I'm worried that I actually *need* this format... at least, the existing MIR test I cargo culted from had this. I really agree with you, as it took me forever to even find the CHECKs in that test!

Anyways, I'll experiment to see if I can get something more readable and still go through the MIR testing layer.

[chandlerc]

Update to tidy up MIR and reflect a syntax change.

[chandlerc]

Ping, love to get a review on the actual functionality here....

llvm/test/CodeGen/X86/bad-tls-fold.mir

19 ↗

(On Diff #132141)

So, I did the experimentation and now (with some helpful pointers from folks on IRC) I understand why this can't be done.

The CHECK-LABEL entries are YAML comments. But the actual CHECK-NOT entries I want are inside a YAML multi-line scalar and thus cannot use the YAML comment syntax and instead must use a comment syntax recognized by the MIR parser for the multi-line scalar. That's why we node to switch to ; in the middle.

I have out-dented them as much as I can to make them visually distinct, but I can't do that all the way because YAML multiline scalars are identified via their indent.

[probinson]

llvm/test/CodeGen/X86/bad-tls-fold.mir

19 ↗

(On Diff #132141)

Syntactically significant whitespace, feh. Even COBOL doesn't do that anymore.
Okay, I really appreciate the effort, thanks!

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp

bool isTwoAddr =
       NumOps > 1 && MI.getDesc().getOperandConstraint(1, MCOI::TIED_TO) != -1;
 
   // FIXME: AsmPrinter doesn't know how to handle
   // X86II::MO_GOT_ABSOLUTE_ADDRESS after folding.
   if (MI.getOpcode() == X86::ADD32ri &&
       MI.getOperand(2).getTargetFlags() == X86II::MO_GOT_ABSOLUTE_ADDRESS)
     return nullptr;
 
+  // GOTTPOFF relocation loads can only be folded into add instructions.
+  // FIXME: Need to exclude other relocations that only support specific
+  // instructions.
+  if (MOs.size() == X86::AddrNumOperands &&
+      MOs[X86::AddrDisp].getTargetFlags() == X86II::MO_GOTTPOFF &&
+      MI.getOpcode() != X86::ADD64rr)
+    return nullptr;
+
   MachineInstr *NewMI = nullptr;
 
   // Attempt to fold any custom cases we have.
   if (MachineInstr *CustomMI =
           foldMemoryOperandCustom(MF, MI, OpNum, MOs, InsertPt, Size, Align))
     return CustomMI;
 
   // Folding a memory location into the two-address part of a two-address

llvm/trunk/test/CodeGen/X86/bad-tls-fold.mir

+# RUN: llc -x mir < %s | FileCheck %s
+--- |
+  target triple = "x86_64-unknown-linux-gnu"
+
+  @x = external global i64
+  @i = external thread_local global i32
+
+  define i32 @or() {
+  entry:
+    ret i32 undef
+  }
+
+  define i32 @and() {
+  entry:
+    ret i32 undef
+  }
+...
+---
+# CHECK-LABEL: or:
+name: or
+alignment: 4
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr64 }
+  - { id: 1, class: gr64 }
+  - { id: 2, class: gr64 }
+  - { id: 3, class: gr64 }
+  - { id: 4, class: gr32 }
+body: |
+  bb.0.entry:
+    %0:gr64 = MOV64rm $rip, 1, $noreg, @x, $noreg :: (load 8)
+    %1:gr64 = OR64ri8 %0, 7, implicit-def dead $eflags
+    %2:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @i, $noreg :: (load 8)
+    %3:gr64 = OR64rr %2, %1, implicit-def dead $eflags
+    %4:gr32 = MOV32rm killed %3, 1, $noreg, 0, $fs :: (load 4)
+  ; CHECK-NOT:  orq {{.*}}GOTTPOFF{{.*}}
+  ;
+  ; What we actually expect:
+  ; CHECK:      movq {{.*}}GOTTPOFF{{.*}}, %[[R:.*]]
+  ; CHECK-NEXT: orq %{{.*}}, %[[R]]
+  ; CHECK-NEXT: movl %fs:(%[[R]]),
+  ;
+  ; CHECK-NOT:  orq {{.*}}GOTTPOFF{{.*}}
+    $eax = COPY %4
+    RET 0, $eax
+
+...
+---
+# CHECK-LABEL: and:
+name: and
+alignment: 4
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr64 }
+  - { id: 1, class: gr64 }
+  - { id: 2, class: gr64 }
+  - { id: 3, class: gr64 }
+  - { id: 4, class: gr32 }
+body: |
+  bb.0.entry:
+    %0:gr64 = MOV64rm $rip, 1, $noreg, @x, $noreg :: (load 8)
+    %1:gr64 = OR64ri8 %0, 7, implicit-def dead $eflags
+    %2:gr64 = MOV64rm $rip, 1, $noreg, target-flags(x86-gottpoff) @i, $noreg :: (load 8)
+    %3:gr64 = AND64rr %2, %1, implicit-def dead $eflags
+    %4:gr32 = MOV32rm killed %3, 1, $noreg, 0, $fs :: (load 4)
+  ; CHECK-NOT:  andq {{.*}}GOTTPOFF{{.*}}
+  ;
+  ; What we actually expect:
+  ; CHECK:      movq {{.*}}GOTTPOFF{{.*}}, %[[R:.*]]
+  ; CHECK-NEXT: andq %{{.*}}, %[[R]]
+  ; CHECK-NEXT: movl %fs:(%[[R]]),
+  ;
+  ; CHECK-NOT:  andq {{.*}}GOTTPOFF{{.*}}
+    $eax = COPY %4
+    RET 0, $eax
+
+...