Page MenuHomePhabricator

[X86] Avoid %fs:(%eax) references in x32 mode
ClosedPublic

Authored by hvdijk on Dec 12 2020, 4:34 AM.

Details

Summary

The ABI explains that %fs:(%eax) zero-extends %eax to 64 bits, and adds
that the TLS base address, but that the TLS base address need not be
at the start of the TLS block, TLS references may use negative offsets.

Diff Detail

Event Timeline

hvdijk created this revision.Dec 12 2020, 4:34 AM
hvdijk requested review of this revision.Dec 12 2020, 4:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2020, 4:34 AM
RKSimon added inline comments.Dec 15 2020, 3:35 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1618

Is there a better name that we can use than NoRegisters?

hvdijk added inline comments.Dec 15 2020, 3:16 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1618

I was thinking of something like AllowSegmentReg initially, but that would be a poor name, since except for X32, segment registers are always allowed. I picked NoRegisters to reflect that when NoRegisters is true, it is known that AM does not and will not refer to any registers other than what matchLoadInAddress does. I now see that that is not entirely true either, as AM may in theory already reference another segment register. NoBaseReg would be slightly more accurate, but still not very clear. Perhaps I should go back to what I originally had and change it to the slightly verbose but pretty clear AllowSegmentRegForX32?

RKSimon added inline comments.Dec 16 2020, 4:48 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1618

Something X32 specific would be better - so AllowSegmentRegForX32 would be fine.

llvm/test/CodeGen/X86/pic.ll
339–340

it doesn't look great to allow add or lea like this - maybe add an additional CHECK-X32-ISEL/CHECK-X32-FAST prefix and check them properly?

hvdijk updated this revision to Diff 312282.Dec 16 2020, 12:29 PM

Rename parameter, split addl/leal check in two.

hvdijk marked 2 inline comments as done.Dec 16 2020, 12:33 PM
hvdijk added inline comments.
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1618

Alright, done.

llvm/test/CodeGen/X86/pic.ll
339–340

My thinking was that the leal is just used as a three-operand add. This test does not check which registers get used, so it seemed odd to me to check that the add's output register is the same as one of its inputs, it seemed like either addl or leal would be fine for either I686 or X32. However, thinking about it some more, there is a difference, and it does make sense to restrict which instructions get accepted by the test: I686 gets addl, because that's the shorter instruction. X32 gets leal, because this prevents clobbering its inputs: for x32, both inputs are reused later on. So, done.

hvdijk marked 2 inline comments as done.Dec 16 2020, 1:03 PM
hvdijk added inline comments.
llvm/test/CodeGen/X86/pic.ll
339–340

Looking at the new diff: for consistency with the other checks, I should have put ({{%.*,%.*}}) here rather than ({{.*,.*}}). I missed this until after I had already submitted the updated version. It's a trivial change that does not affect anything, but will update nonetheless.

hvdijk updated this revision to Diff 312288.Dec 16 2020, 1:04 PM

Be consistent in CHECK lines.

RKSimon accepted this revision.Dec 16 2020, 2:35 PM

LGTM

This revision is now accepted and ready to land.Dec 16 2020, 2:35 PM
This revision was landed with ongoing or failed builds.Dec 16 2020, 2:40 PM
This revision was automatically updated to reflect the committed changes.