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

[Jim Grosbach]

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18070
@@ -18062,1 +18069,3 @@
+          }
+
           SDValue ANDed = DAG.getNode(ISD::AND, DL, IntVT, OnesOrZeroesI,
----------------
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.

================
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),
----------------
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.

================
Comment at: lib/Target/X86/X86ISelLowering.h:158
@@ +157,3 @@
+      /// to a 32-bit GPR.
+      SSE_MOVD2W,
+
----------------
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.


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