[PATCH] D25980: [PowerPC] Improvements for BUILD_VECTOR Vol. 2

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10521
 
+/// combineBVOfFpToIntToFpToIntOfBV - If this node is a build_vector of
+/// fp-to-int conversions, reduce it to a fp-to-int of a build_vector
----------------
kbarton wrote:
> Please convert this to a doxygen-style comment.
OK, I'll do that.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10527
+SDValue
+PPCTargetLowering::combineBVOfFpToIntToFpToIntOfBV(SDNode *N,
+                                                   DAGCombinerInfo &DCI) const {
----------------
kbarton wrote:
> I know this name is descriptive of what the function does, but it's very difficult to read. 
> Is there another name possible? Maybe something containing transpose, exchange, swap or refactor?
OK I'll change it to
`combineElementTruncationToVectorTruncation`
I think it sufficiently conveys what the function does and it reads quite nicely as well.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10552
+    EVT TargetVT = N->getValueType(0);
+    for (int i = 0, e = N->getNumOperands(); i < e; i++) {
+      if (N->getOperand(i).getOpcode() != PPCISD::MFVSR)
----------------
kbarton wrote:
> Use ++i not i++
OK.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10569
+    // Now that we know we have the right type of node, get its operands
+    for (int i = 0, e = N->getNumOperands(); i < e; i++) {
+      SDValue In = N->getOperand(i).getOperand(0);
----------------
kbarton wrote:
> ++i
OK. Sorry, just a habit to use pre-increment for class types and post-increment for primitive types. But I'll change it.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10586
+    unsigned Opcode;
+    if(FirstConversion == PPCISD::FCTIDZ ||
+       FirstConversion == PPCISD::FCTIWZ)
----------------
kbarton wrote:
> space between if (
Good catch. Will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D25980