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

[Harald van Dijk via Phabricator via llvm-commits]

hvdijk marked 2 inline comments as done.
hvdijk added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1618
+bool X86DAGToDAGISel::matchLoadInAddress(LoadSDNode *N, X86ISelAddressMode &AM,
+                                         bool NoRegisters) {
   SDValue Address = N->getOperand(1);
----------------
RKSimon wrote:
> hvdijk wrote:
> > RKSimon wrote:
> > > Is there a better name that we can use than NoRegisters?
> > 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`?
> Something X32 specific would be better - so AllowSegmentRegForX32 would be fine.
Alright, done.


================
Comment at: llvm/test/CodeGen/X86/pic.ll:339
 ; CHECK-X32-DAG:	movl	%fs:0,
-; CHECK:	addl
+; CHECK:	{{addl|leal \(%.*,%.*\),}}
 ; CHECK-I686:	movl	tlsptrie at GOTNTPOFF(
----------------
RKSimon wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93158/new/

https://reviews.llvm.org/D93158