[PATCH] D110342: [x86] convert logic-of-FP-compares to FP logic-of-vector-compares

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 3 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45453
 /// If both input operands of a logic op are being cast from floating point
-/// types, try to convert this into a floating point logic node to avoid
-/// unnecessary moves from SSE to integer registers.
+/// types or FP compares, try to convert this into a floating-point logic node
+/// to avoid unnecessary moves from SSE to integer registers.
----------------
pengfei wrote:
> Nit, why use hyphen here but leave space above?
I just overlooked the first one - will fix it.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45510
+  SDValue Logic = DAG.getNode(N->getOpcode(), DL, BoolVecVT, Setcc0, Setcc1);
+  return DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, VT, Logic, ZeroIndex);
 }
----------------
pengfei wrote:
> RKSimon wrote:
> > Do we need to check/assert that VT is MVT::i1 ?
> I think it's Ok since the logic operands come from setcc directly. Do we need to handle `(logic (zext (setcc ...`?
I haven't been able to find an example where we have the pattern late only, but the result of x86 scalar setcc could be i8 after some transforms. 

There's no difference on the existing tests if I add the check for i1, so I'll do that.

Also, the pattern where we have sext/zext between the logic op and setcc is already folded away by DAGCombiner (the cast is moved after the logic), so I don't think we need to worry about that case.


================
Comment at: llvm/test/CodeGen/X86/fcmp-logic.ll:242-258
 ; SSE2-LABEL: olt_olt_and_f32_f64:
 ; SSE2:       # %bb.0:
 ; SSE2-NEXT:    ucomiss %xmm0, %xmm1
 ; SSE2-NEXT:    seta %cl
 ; SSE2-NEXT:    ucomisd %xmm2, %xmm3
 ; SSE2-NEXT:    seta %al
 ; SSE2-NEXT:    andb %cl, %al
----------------
pengfei wrote:
> Should we add a common `CHECK` for them?
It's the "v" in the mnemonics that makes these different. Do we have a scrubber option in the script for that?


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

https://reviews.llvm.org/D110342