[PATCH] Don't generate 64-bit movd after cmpneqsd in 32-bit mode (PR19059)

[Hans Wennborg]

  Thank you very much for the comments.


================
Comment at: lib/Target/X86/X86InstrSSE.td:4824
@@ +4823,3 @@
+
+let isCodeGenOnly = 1 in {
+def MOVSS2Irr : S2I<0x7E, MRMDestReg, (outs GR32:$dst), (ins FR64:$src),
----------------
Jim Grosbach wrote:
> Which instruction does this share an encoding with? We should instead be using a Pat<> which references that. Multiple instruction definitions for the same encoding is something we're actively moving away from. We should avoid adding new ones, at the least.
It shares encoding with MOVSS2DIrr a couple of lines above. The only difference is the type of the "ins".

MOVSS2DIrr expects a FR32 source, but my instruction has a FR64 source. Is it still possible to use a Pat<> instead? If so, do you have any pointers to how I can "convert" the register to make it work?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18070
@@ -18062,1 +18069,3 @@
+          }
+
           SDValue ANDed = DAG.getNode(ISD::AND, DL, IntVT, OnesOrZeroesI,
----------------
Jim Grosbach wrote:
> Hans Wennborg wrote:
> > Jim Grosbach wrote:
> > > This seems very strange. Why do we not want to perform the operations in 64-bits here? It's still 64-bit data whether we're in 64-bit mode or not, right?
> > > 
> > > Can you elaborate a bit on what's actually going wrong here?
> > (I'm new to this so please correct me if I seem confused here..)
> > 
> > > This seems very strange. Why do we not want to perform the operations in 64-bits 
> > > here? It's still 64-bit data whether we're in 64-bit mode or not, right?
> > 
> > This runs after the DAG has been legalized. If we generate a 64-bit ISD::BITCAST node, we'll select an instruction that moves to a 64-bit integer register, e.g. "movd %xmm1, %rax", which obviously doesn't work well when targeting 32-bit.
> OK, that makes sense. The operation itself is still 64-bit, though, as currently phrased in the code. The proposed patch changes that to only operate on the lower 32-bits instead. Given the operations being performed, that seems OK, but it is a different operation than the BITCAST that's done in 64-bit mode and there should at minimum be a very clear comment about why it's a reasonable thing to do. Put another way, this is somewhat tricky code that's poorly commented currently and I'd like us to be careful to make that better not worse when we change it.
I'll put in a better comment.

================
Comment at: lib/Target/X86/X86ISelLowering.h:158
@@ +157,3 @@
+      /// to a 32-bit GPR.
+      SSE_MOVD2W,
+
----------------
Jim Grosbach wrote:
> Is there no way to express this operation with the existing ISD nodes? The way X86 represents these registers is really strange, so I'll believe it if not, but target ISD nodes inhibit other target independent patterns from matching, so it's best to avoid them if possible.
We need a way to express "grab the low 32 bits of a 64-bit float". The only way I could think of would be 64-bit bitcast + truncate to 32 bits. I could match the instruction against that, but I figured it would be wrong to generate a 64-bit int value in the sdag after legalization, even if we know we can select it.


http://llvm-reviews.chandlerc.com/D3009