[PATCH] D36454: [X86] Changes to extract Horizontal addition operation for AVX-512.

[Jatin Bhateja via Phabricator via llvm-commits]

jbhateja added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:30189
+      SDValue NewEltIdx =
+          DAG.getConstant((ExtractedElt - StartIdx), dl, MVT::i32);
+
----------------
craig.topper wrote:
> I think element indices should use DAG.getIntPtrConstant to make the type correct. It's not supposed to be i32.
Yes this will be fixed.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:33793
 
+  bool ExtractLHSSubVec = false;
+  ConstantSDNode *CSD = nullptr;
----------------
craig.topper wrote:
> Can we teach combineExtractSubvector to narrow an extract of an add by extracting from the inputs? Then we shouldn't need any change here because we'll already have the narrow add?
I think each Node specific combiner should look at pattern starting from itself.

i.e. combineExtractSubvector could be taught to remove extract_subvector from a vector shuffle (if has only one use) thus producing a smaller vector shuffle that will be generic change.

Looking for opportunity like you mentioned "extract of an add by extracting from input" appears as a specialization.

As of now narrowing down has be done genrically at following two places
1/ Extract_vector_elt : It looks at its input vector and try scaling down if possible. 

2/ Narrow down binary operation (add/sub/fadd/fsub) [this is implicitly doing whay your comment says).


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:33886
   // Check that the masks correspond to performing a horizontal operation.
-  for (unsigned l = 0; l != NumElts; l += NumLaneElts) {
+  for (unsigned L = 0; L != NumElts; L += NumLaneElts) {
     for (unsigned i = 0; i != NumLaneElts; ++i) {
----------------
craig.topper wrote:
> Why is this variable now upper case?
For better visual clarity of variable name.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:33936
 
+  auto NewOpcode = IsFadd ? X86ISD::FHADD : X86ISD::FHSUB;
+
----------------
craig.topper wrote:
> Why was this moved out of the if.
This will be fixed.


https://reviews.llvm.org/D36454