Code style changes (VectorUtils.cpp)

[delena]

Summary

Changes in code style following the mail from James Molloy:

Hi Elena,

Firstly, it looks like your patch *fixes* a bug: the previous code didn't check that the InsertVectorElt it found was actually inserting into lane zero. Could a testcase be added to check this?

[Elena] This function will be used in the upcoming commit ( http://reviews.llvm.org/D11121 ). I'll add more tests there.

Secondly there are some coding style problems here

+llvm::ConstantDataVector *CV = dyn_cast<llvm::ConstantDataVector>(V);
+if (CV)
+return CV->getSplatValue();

(And several other places): This would be more idiomatically written as:

if (auto *CV = dyn_cast<ConstantDataVector>(V))

return CV->getSplatValue();

There are other places where you needlessly use the llvm:: namespace prefix too.

+ // All-zero (our undef) shuffle mask elements.

s/our/or.

for (int i : ShuffleInst->getShuffleMask())

We idiomatically use capitals for our loop induction variables.

if (i != 0 && i != -1)

The function should have a comment stating that it will only detect splats that insert into the zero'th element of the left-hand vector (idiomatic splats). The function could easily be made to detect non-idiomatic splats too, but as long as the function says it explicitly that's good enough, I think.

llvm::InsertElementInst *InsertEltInst =

dyn_cast<llvm::InsertElementInst>(ShuffleInst->getOperand(0));

You don't need to use llvm::, and you can use auto to shorten the line.

Cheers,

James

[jmolloy]

Apart from my one comment, LGTM.

Thankyou for making these changes, Elena!

../lib/Analysis/VectorUtils.cpp

418 ↗

(On Diff #33532)

You don't need llvm:: here.

llvm/trunk/lib/Analysis/VectorUtils.cpp

if (Constant *Elt = Con->getAggregateElement(EltNo))
       if (Elt->isNullValue())
         return findScalarElement(Val, EltNo);
 
   // Otherwise, we don't know.
   return nullptr;
 }
 
 /// \brief Get splat value if the input is a splat vector or return nullptr.
-/// The value may be extracted from a splat constants vector or from
-/// a sequence of instructions that broadcast a single value into a vector.
+/// This function is not fully general. It checks only 2 cases:
+/// the input value is (1) a splat constants vector or (2) a sequence
+/// of instructions that broadcast a single value into a vector.
+///
 llvm::Value *llvm::getSplatValue(Value *V) {
-  llvm::ConstantDataVector *CV = dyn_cast<llvm::ConstantDataVector>(V);
-  if (CV)
+  if (auto *CV = dyn_cast<ConstantDataVector>(V))
     return CV->getSplatValue();
-  llvm::ShuffleVectorInst *ShuffleInst = dyn_cast<llvm::ShuffleVectorInst>(V);
+
+  auto *ShuffleInst = dyn_cast<ShuffleVectorInst>(V);
   if (!ShuffleInst)
     return nullptr;
-  // All-zero (our undef) shuffle mask elements.
-  for (int i : ShuffleInst->getShuffleMask())
-    if (i != 0 && i != -1)
+  // All-zero (or undef) shuffle mask elements.
+  for (int MaskElt : ShuffleInst->getShuffleMask())
+    if (MaskElt != 0 && MaskElt != -1)
       return nullptr;
   // The first shuffle source is 'insertelement' with index 0.
-  llvm::InsertElementInst *InsertEltInst =
-    dyn_cast<llvm::InsertElementInst>(ShuffleInst->getOperand(0));
+  auto *InsertEltInst =
+    dyn_cast<InsertElementInst>(ShuffleInst->getOperand(0));
   if (!InsertEltInst || !isa<ConstantInt>(InsertEltInst->getOperand(2)) ||
       !cast<ConstantInt>(InsertEltInst->getOperand(2))->isNullValue())
     return nullptr;
 
   return InsertEltInst->getOperand(1);
 }