Use PC-relative address for x32 TLS address

[hvdijk]

Summary

Since x32 supports PC-relative address, it shouldn't use EBX for TLS
address. Instead of checking N.getValueType(), we should check
Subtarget->is32Bit(). This fixes PR 22676.

[mgrang]

Could you please upload your patch with more context? (Preferably the full src file).

lib/Target/X86/X86ISelDAGToDAG.cpp

1682 ↗

(On Diff #45704)

Is there any specific reason why the if-else clauses have been reversed? i.e: why are we checking for 64 bit first? Can we still do this:

if (Subtarget->is32Bit()) {

  AM.Scale = 1;
  AM.IndexReg = CurDAG->getRegister(X86::EBX, MVT::i32);
}

else {

AM.IndexReg = CurDAG->getRegister(0, N.getValueType());

}

[hjl.tools]

Use Subtarget->is32Bit()) instead. The full function now is:

/// This is only run on TargetGlobalTLSAddress nodes.
bool X86DAGToDAGISel::selectTLSADDRAddr(SDValue N, SDValue &Base,

                                      SDValue &Scale, SDValue &Index,
                                      SDValue &Disp, SDValue &Segment) {
assert(N.getOpcode() == ISD::TargetGlobalTLSAddress);
const GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(N);

X86ISelAddressMode AM;
AM.GV = GA->getGlobal();
AM.Disp += GA->getOffset();
AM.Base_Reg = CurDAG->getRegister(0, N.getValueType());
AM.SymbolFlags = GA->getTargetFlags();

if (Subtarget->is32Bit()) {
  AM.Scale = 1;
  AM.IndexReg = CurDAG->getRegister(X86::EBX, MVT::i32);
} else {
  AM.IndexReg = CurDAG->getRegister(0, N.getValueType());
}

getAddressOperands(AM, SDLoc(N), Base, Scale, Index, Disp, Segment);
return true;

}

[mgrang]

This comment was removed by mgrang.

[• rafael]

testcase?

[hjl.tools]

Since llvm doesn't support TLS for x32:

[hjl@gnu-mic-2 X86]$ cat x32-tls-1.ll
@x = thread_local global i32 0, align 4

define i32* @get_x() {
entry:

ret i32* @x

}
[hjl@gnu-mic-2 X86]$ /export/build/gnu/llvm-clang/build-x86_64-linux/bin/llc -relocation-model=pic -mtriple=x86_64-linux-gnux32 < x32-tls-1.ll
.text
.file "<stdin>"
LLVM ERROR: Cannot select: 0x29507a0: ch,glue = X86ISD::TLSADDR 0x28fc140, TargetGlobalTLSAddress:i32<i32* @x> 0 [TF=7]

0x2950670: i32 = TargetGlobalTLSAddress<i32* @x> 0 [TF=7]

In function: get_x
[hjl@gnu-mic-2 X86]$

I don't know how to create a test for this change.

[GreenReaper]

If https://bugs.llvm.org/show_bug.cgi?id=38932 is indeed a duplicate of the bug filed for this, https://bugs.llvm.org/show_bug.cgi?id=22676 then it seems the filer of the duplicate has provided a simple test case.

I think the bug also popped up at https://bugs.llvm.org/show_bug.cgi?id=26472 judging by the function.

[RKSimon]

reverse ping - interest in x32 has resurfaced recently

[hjl.tools]

My old x32 patches are at https://github.com/hjl-tools/llvm-x32-old/tree/hjl/x32/2016-09-01

[hvdijk]

In D16474#335360, @hjl.tools wrote:

LLVM ERROR: Cannot select: 0x29507a0: ch,glue = X86ISD::TLSADDR 0x28fc140, TargetGlobalTLSAddress:i32<i32* @x> 0 [TF=7]

This still happens with current LLVM. If we can push the fix for that first (presumably you fixed this already on your old branch), we could then include your test case in this diff.

[hjl.tools]

In D16474#2421552, @hvdijk wrote:

In D16474#335360, @hjl.tools wrote:

LLVM ERROR: Cannot select: 0x29507a0: ch,glue = X86ISD::TLSADDR 0x28fc140, TargetGlobalTLSAddress:i32<i32* @x> 0 [TF=7]

This still happens with current LLVM. If we can push the fix for that first (presumably you fixed this already on your old branch), we could then include your test case in this diff.

Please feel free to pick up my x32 patches and submit them to the current LLVM if they are still applicable. Thanks.

[RKSimon]

@hvdijk Are you in a position to commandeer this phab revision and complete @hjl.tools's work?

[hvdijk]

In D16474#2421640, @RKSimon wrote:

@hvdijk Are you in a position to commandeer this phab revision and complete @hjl.tools's work?

Yes, I think so. I took a look at the ISel failure, but that cannot be fixed independently, fixing that by itself results in invalid machine instructions because of the use of $ebx without any definition, exactly what this diff fixes. I would say this diff should be updated to apply to current LLVM and then committed without a test. This is a trivial change: we still need to change the condition to if (Subtarget->is32Bit()) exactly as in this diff, but the else block is no longer there after r359318, it has been moved into getAddressOperands in a way that already respects N's type, so that part of the diff can just be removed. Then, once that is in, I can create a new diff for the ISel failure and include a test in that. Does that sound okay?

[RKSimon]

Makes sense to me - let's get the patch updated and see if anyone has any objections.

[hvdijk]

Update to current LLVM.

[RKSimon]

LGTM - cheers

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

bool X86DAGToDAGISel::selectTLSADDRAddr(SDValue N, SDValue &Base,
   assert(N.getOpcode() == ISD::TargetGlobalTLSAddress);
   const GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(N);
 
   X86ISelAddressMode AM;
   AM.GV = GA->getGlobal();
   AM.Disp += GA->getOffset();
   AM.SymbolFlags = GA->getTargetFlags();
 
-  MVT VT = N.getSimpleValueType();
-  if (VT == MVT::i32) {
+  if (Subtarget->is32Bit()) {
     AM.Scale = 1;
     AM.IndexReg = CurDAG->getRegister(X86::EBX, MVT::i32);
   }
 
+  MVT VT = N.getSimpleValueType();
   getAddressOperands(AM, SDLoc(N), VT, Base, Scale, Index, Disp, Segment);
   return true;
 }
 
 bool X86DAGToDAGISel::selectRelocImm(SDValue N, SDValue &Op) {
   // Keep track of the original value type and whether this value was
   // truncated. If we see a truncation from pointer type to VT that truncates
   // bits that are known to be zero, we can use a narrow reference.