[PATCH] D11121: Optimization for Gather/Scatter with uniform base

[Ahmed Bougacha]

ab added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3083
@@ -3082,2 +3082,3 @@
 
+// Get a unform base for the Gather/Scatter input.
 // Gather/scatter receive a vector of pointers.
----------------
unform -> uniform

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3084
@@ -3083,2 +3083,3 @@
+// Get a unform base for the Gather/Scatter input.
 // Gather/scatter receive a vector of pointers.
 // This vector of pointers may be represented as a base pointer + vector of 
----------------
receive -> take, perhaps?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3085
@@ -3083,2 +3084,3 @@
 // Gather/scatter receive a vector of pointers.
 // This vector of pointers may be represented as a base pointer + vector of 
+// indices. Usually, the vector of pointers comes from a 'getelementptr'
----------------
Perhaps IR examples for both representations might be helpful?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3111-3112
@@ -3108,16 +3110,4 @@
     Base = SDB->getValue(Ptr);
-  else if (SDB->findValue(ShuffleInst)) {
-    SDValue ShuffleNode = SDB->getValue(ShuffleInst);
-    SDLoc sdl = ShuffleNode;
-    Base = DAG.getNode(
-        ISD::EXTRACT_VECTOR_ELT, sdl,
-        ShuffleNode.getValueType().getScalarType(), ShuffleNode,
-        DAG.getConstant(0, sdl, TLI.getVectorIdxTy(DAG.getDataLayout())));
-    SDB->setValue(Ptr, Base);
   }
-  else
-    return false;
-
-  Value *IndexVal = Gep->getOperand(1);
-  if (SDB->findValue(IndexVal)) {
-    Index = SDB->getValue(IndexVal);
+  else {
+    // The base is a vector. But may be this vector is splat.
----------------
else on the same line?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3116-3117
@@ +3115,4 @@
+    ShuffleVectorInst *ShuffleInst = dyn_cast<ShuffleVectorInst>(GepBasePtr);
+    if (!ShuffleInst || !ShuffleInst->getMask()->isNullValue() ||
+        !isa<InsertElementInst>(ShuffleInst->getOperand(0)))
+      return false;
----------------
Should we check that the insertelement index is 0?  (or even generalize and check that the broadcast and insertelement have the same index, though that sounds unnecessary)

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3121
@@ +3120,3 @@
+    Ptr = cast<InsertElementInst>(ShuffleInst->getOperand(0))->getOperand(1);
+    // Check if the Ptr is inside current basic block.
+    // If not, look for the shuffle instruction
----------------
inside -> inside the

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3129
@@ +3128,3 @@
+      EVT IdxVT = TLI.getVectorIdxTy(DAG.getDataLayout());
+      Base = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, sdl, 
+                         ShuffleNode.getValueType().getScalarType(),
----------------
unnecessary end-of-line whitespace?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3140
@@ +3139,3 @@
+  if (SExtInst* Sext = dyn_cast<SExtInst>(IndexVal)) {
+    IndexVal = Sext->getOperand(0);
+    if (SDB->findValue(IndexVal))
----------------
I'm a bit uneasy with changing IndexVal outside the if():  when the latter fails,  IndexVal will be incorrect for subsequent users.

Not sure it's better, but what about something like:
    Value *OrigIndexVal = ...;
    if (SDB->findValue(OrigIndexVal)) {
      IndexVal = OrigIndexVal;
      ...
    }

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3146
@@ +3145,3 @@
+    unsigned GEPWidth = Gep->getType()->getVectorNumElements();
+    MVT VT = MVT::getVectorVT(Index.getValueType().getSimpleVT(), GEPWidth);
+    SmallVector<SDValue, 16> Ops(GEPWidth, Index);
----------------
Why not EVT?


Repository:
  rL LLVM

http://reviews.llvm.org/D11121