[PATCH] D116928: [LoopVectorize] Support epilogue vectorisation of loops with reductions

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes, this looks better! I've left a few more comments.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:561
 
+  // Returns the resume value for a reduction if set, otherwise returns
+  // nullptr.
----------------
nit:
  s/resume value for a reduction if set, otherwise returns nullptr./
    resume value (bc.merge.rdx) for a reduction as generated by fixReduction./

In the implementation of `getResumeValue`, I'd also add an assert that the record exists to avoid it being queried when the information is not yet available, and also to ensure the record isn't missing for whatever reason.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:563
+  // nullptr.
+  Value *getResumeValue(const RecurrenceDescriptor *RdxDesc);
+
----------------
This should return a PHI node, because it can only ever be a PHINode.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:834
+  // correct start value of reduction PHIs when vectorizing the epilogue.
+  SmallMapVector<const RecurrenceDescriptor *, Value *, 4> ResumeValues;
 };
----------------
PHINode


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1231
+Value *
+InnerLoopVectorizer::getResumeValue(const RecurrenceDescriptor *RdxDesc) {
+  if (ResumeValues.find(RdxDesc) != ResumeValues.end())
----------------
nit: maybe just pass the RecurrenceDescriptor in as const reference? (because in most places that calls this function the recurrence descriptor is a reference, not a pointer).


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1232-1234
+  if (ResumeValues.find(RdxDesc) != ResumeValues.end())
+    return ResumeValues[RdxDesc];
+  return nullptr;
----------------
You could resume the iterator from `find` to return the result, rather than having to search through ResumeValues twice.

If you assert that the value must be available, you can do:

  auto It = ResumeValues.find(RdxDesc);
  assert(It != ResumeValues.end() && "Message");
  return *It;


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4339
+
+  // Set the resume value for this reduction
+  if (ResumeValues.find(&RdxDesc) != ResumeValues.end())
----------------
nit: maybe move to line 4317 so that it's together with the creation of BCBlockPhi?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4340-4341
+  // Set the resume value for this reduction
+  if (ResumeValues.find(&RdxDesc) != ResumeValues.end())
+    ResumeValues[&RdxDesc] = cast<Value>(BCBlockPhi);
+  else
----------------
does this ever happen? I'd expect ResumeValues not to contain a PHINode for the resume value of this reduction descriptor yet.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4343
+  else
+    ResumeValues.insert({&RdxDesc, cast<Value>(BCBlockPhi)});
 }
----------------
a PHINode is a Value


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8270-8273
+  // incoming values here and move the Phi into the preheader:
+  // - Update the predecessor of vec.epilog.iter.check -> vec.epilog.iter.check
+  // - Remove incoming values from SafetyCheck blocks & the epilogue
+  //   iteration count check block
----------------
nit: the highlighted bit seems redundant, because the code below says as much.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116928/new/

https://reviews.llvm.org/D116928