[PATCH] D9822: Reducing the costs of cast instructions to enable more vectorization of smaller types in LoopVectorize

[James Molloy]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Sam,

I've got a bunch of typographical changes, but the biggest problem is still the unbounded recursion.

Cheers,

James


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1170
@@ +1169,3 @@
+  /// inserted in its place.
+  void InsertConfirmedNarrow(Instruction *I, Type *NarrowTy, unsigned VF);
+
----------------
All these functions should start with a lowercase letter.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3559
@@ +3558,3 @@
+
+          Type *Ty = ClampedVecTys[it]->getScalarType();
+          VectorType *VecTy = VectorType::get(Ty, VF);
----------------
I'm confused as to why we need ->getScalarType() here - surely this can't be a vector type by definition, as it hasn't yet been vectorized?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5110
@@ +5109,3 @@
+
+  llvm_unreachable("one type should be integer!");
+  return T0;
----------------
In what case can you get here and one of the types not be an integer?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5122
@@ +5121,3 @@
+      if (Largest == NarrowTy) {
+        CandidateNarrowInstrs[VF].erase(I);
+        CandidateNarrowInstrs[VF].insert(std::make_pair(I, NarrowTy));
----------------
I think it'd be easier if you just did:

    CandidateNarrowInstrs[VF][I] = Largest;

To remove a bunch of useless code.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5125
@@ +5124,3 @@
+      }
+    } else
+      CandidateNarrowInstrs[VF].insert(std::make_pair(I, NarrowTy));
----------------
Braces around else.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5137
@@ +5136,3 @@
+    if (Largest == NarrowTy) {
+      NarrowInstrs[VF].erase(I);
+      NarrowInstrs[VF].insert(std::make_pair(I, NarrowTy));
----------------
Again, use std::map::operator[] rather than ::erase and ::insert.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5198
@@ +5197,3 @@
+    if (Instruction *NextOp = dyn_cast<Instruction>(Opr)) {
+      // If we find an already confirmed operand, grab its value.
+      if (NarrowInstrs[VF].count(NextOp))
----------------
You're not actually grabbing any value here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5199
@@ +5198,3 @@
+      // If we find an already confirmed operand, grab its value.
+      if (NarrowInstrs[VF].count(NextOp))
+        ++NumConfirmed;
----------------
Here and elsewhere: if statements should either be one-liners or multi-liners. Please don't mix one-liners and braces.

    if (a)
      b;
    else
      c;  // GOOD

    if (a) {
      b; c;
    } else
      d; // BAD

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5203
@@ +5202,3 @@
+        // Need to confirm the type of this operand
+        if (ConfirmNarrowChain(NextOp, VF, NarrowTy))
+          ++NumConfirmed;
----------------
This is unboundedly recursive.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5245
@@ +5244,3 @@
+  // bound the chains of operations.
+  unsigned Opc = I->getOpcode();
+  if (Opc != Instruction::Mul &&
----------------
Please use a switch here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5264
@@ +5263,3 @@
+
+  const APInt i8MaxValue = APInt::getMaxValue(8);
+  const APInt i16MaxValue = APInt::getMaxValue(16);
----------------
Capital letter.


http://reviews.llvm.org/D9822