[PATCH] D13551: [LoopVectorize] Shrink integer operations into the smallest type possible

[James Molloy via llvm-commits]

jmolloy added a comment.

Hi Silviu,

Thanks for the comments, I'll address them all tomorrow. I don't have LNT or spec numbers right now, but I can get them for you tomorrow.

Cheers,

James


================
Comment at: lib/Analysis/VectorUtils.cpp:441
@@ +440,3 @@
+
+DenseMap<Instruction*, uint64_t> llvm::computeMinimumValueSizes(
+  ArrayRef<BasicBlock*> Blocks, DemandedBits &DB,
----------------
sbaranga wrote:
> Returning possibly large structs is not ideal. Would it be better for this function to take a reference to a map instead?
Ah, but we're in C++11 now! I did have to check on google that returning a class will invoke the move constructor... apparently so!

================
Comment at: lib/Analysis/VectorUtils.cpp:549
@@ +548,3 @@
+    
+    uint64_t MinBW = 64 - llvm::countLeadingZeros(LeaderDemandedBits);
+    // Round up to a power of 2
----------------
sbaranga wrote:
> Should be sizeof(LeaderDemandedBits) instead of 64.
Agreed. I will make this change.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3146
@@ +3145,3 @@
+  // later and will remove any ext/trunc pairs.
+  //
+  for (auto &KV : MinBWs) {
----------------
sbaranga wrote:
> Is there any way of doing this without leaving the ext/trunc pairs?
> 
> Maybe instead of generating a trunc look if we're doing it for an ext and if so use the ext operand instead.
Yeah, I suppose that would be neater than relying on InstCombine. I'll take a look at this.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3783
@@ -3653,2 +3782,3 @@
       VectorParts &B = getVectorValue(it->getOperand(1));
+
       for (unsigned Part = 0; Part < UF; ++Part) {
----------------
sbaranga wrote:
> Stray whitespace change?
Woops, will fix.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5303
@@ -5169,3 +5302,3 @@
   case Instruction::FCmp: {
     Type *ValTy = I->getOperand(0)->getType();
     VectorTy = ToVectorTy(ValTy, VF);
----------------
sbaranga wrote:
> I think you wouldn't get the correct cost here (for ICmp)?
Ouch, you're right. I'll fix that.


Repository:
  rL LLVM

http://reviews.llvm.org/D13551