[PATCH] D21703: Scalarizer: Support scalarizing intrinsics

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:110
@@ +109,3 @@
+  CallInst &CI;
+};
+
----------------
It is not clear to me what is this doing?
(Why do you need the CI member for instance)

Looks like you mimic'd `struct BinarySplitter`, but simplifications make it moot.

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:420
@@ +419,3 @@
+
+bool Scalarizer::splitCall(CallInst &CI, const CallSplitter &Split) {
+  VectorType *VT = dyn_cast<VectorType>(CI.getType());
----------------
Document the function

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:439
@@ +438,3 @@
+
+  Scattered.resize(NumArgs);
+  for (unsigned I = 0; I != NumArgs; ++I) {
----------------
Is this dead code?

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:445
@@ +444,3 @@
+      Scattered[I] = scatter(&CI, CI.getOperand(I));
+      assert(Scattered[I].size() == NumElems && "mismatched call operands");
+    }
----------------
This seems to assume that every vector operand must have the same number of elements as the returned type?

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:447
@@ +446,3 @@
+    }
+  }
+
----------------
Would this loop be equivalent to: 

```
for (auto &Operand : I->operands()) {
  if (!Operand.getType()->isVectorTy())
    ScalarOperands[I] = CI.getOperand(I);
  else {
    Scattered[I] = scatter(&CI, CI.getOperand(I));
    assert(Scattered[I].size() == NumElems && "mismatched call operands");
  }
}
```

I'm not sure about `hasVectorInstrinsicScalarOpd` though...

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:453
@@ +452,3 @@
+  Res.resize(NumElems);
+  ScalarCallOps.resize(NumArgs);
+
----------------
I think you could pass the initial size to the ctor.
It may even be better to call `reserve()` on `ScalarCallOps` though as the next thing is clearing it.
I'd probably use reserve on `Res` as well and use `push_back` in the loop below.


================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:470
@@ +469,3 @@
+                      CI.getName() + ".i" + Twine(Elem));
+  }
+
----------------
A comment above the loop can be appreciated.


http://reviews.llvm.org/D21703