Page MenuHomePhabricator

Use PC-relative address for x32 TLS address
ClosedPublic

Authored by hvdijk on Jan 22 2016, 10:45 AM.

Details

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.

Diff Detail

Event Timeline

hjl.tools updated this revision to Diff 45704.Jan 22 2016, 10:45 AM
hjl.tools retitled this revision from to Use PC-relative address for x32 TLS address .
hjl.tools updated this object.
hjl.tools added reviewers: zansari, DavidKreitzer.
hjl.tools added a subscriber: llvm-commits.
mgrang added a subscriber: mgrang.Jan 22 2016, 11:32 AM

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

lib/Target/X86/X86ISelDAGToDAG.cpp
1682

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 updated this revision to Diff 45727.Jan 22 2016, 12:27 PM
hjl.tools updated this object.

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;

}

This comment was removed by mgrang.

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.

hjl.tools updated this revision to Diff 56457.May 6 2016, 1:55 PM
hjl.tools updated this object.
hjl.tools added a subscriber: qcolombet.

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.

reverse ping - interest in x32 has resurfaced recently

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.

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.

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

@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?

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

hvdijk commandeered this revision.Nov 29 2020, 2:11 PM
hvdijk updated this revision to Diff 308231.
hvdijk edited reviewers, added: hjl.tools; removed: hvdijk.
hvdijk retitled this revision from Use PC-relative address for x32 TLS address to Use PC-relative address for x32 TLS address.
hvdijk edited the summary of this revision. (Show Details)
hvdijk set the repository for this revision to rG LLVM Github Monorepo.

Update to current LLVM.

Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2020, 2:12 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon accepted this revision.Dec 2 2020, 8:18 AM

LGTM - cheers

This revision is now accepted and ready to land.Dec 2 2020, 8:18 AM
This revision was automatically updated to reflect the committed changes.