[PATCH] D56011: [x86] lower extracted fadd/fsub to horizontal vector math

[Sanjay Patel via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18339
+  SDValue RHS = Op.getOperand(1);
+  if (!LHS.hasOneUse() || !RHS.hasOneUse() || !Subtarget.hasSSE3())
+    return Op;
----------------
RKSimon wrote:
> Do both ops have to be one use for this to be useful?
It's not as clearly a win, but I'll ease that constraint, and we can decide if it looks better.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18358
+    std::swap(LExtIndex, RExtIndex);
+  if (LExtIndex != 0 || RExtIndex != 1)
+    return Op;
----------------
RKSimon wrote:
> Is it useful to support any suitable neighbouring pairs of extracted values?
Yes, but arguably the other patterns are less likely (the 0/1 case is what I'm seeing from adjusting the expansion of horizontal reductions). Let me make that a TODO.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18364
+  unsigned BitWidth = VecVT.getSizeInBits();
+  if (BitWidth != 128 && BitWidth != 256)
+    return Op;
----------------
RKSimon wrote:
> Is this important? I think its what you were asking about AVX512 (which doesn't have HADDPS). I'd start by adding avx512f/avx512vl tests to haddsub-undef.ll
Right - we would want to treat 512-bit extract identically to 256-bit. Ie, it doesn't really matter that there's no 512 version of haddps, we want to narrow it to a 128-bit vector first either way.


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

https://reviews.llvm.org/D56011