[PATCH] D56075: [GuardWidening] Support widening of explicitly expressed guards

[Philip Reames via Phabricator via llvm-commits]

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

A few minor tweaks left, but rapidly approaching ready to land.



================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:402
+    if (i == (e - 1) && (CurBB->getTerminator() != GuardInst ||
+                         isExplicitGuard(CurBB->getTerminator()))) {
       // Corner case: make sure we're only looking at guards strictly dominating
----------------
mkazantsev wrote:
> reames wrote:
> > Not sure this check is needed?
> The logic here is a bit messy, I'll try to find a way to refactor the code around to go without it if possible.
See my response to one of your NFC cleanups on how this can be removed entirely.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:535
 
+  Value *WidenableCondition = nullptr;
+  if (InsertPt && isGuardAsWidenableBranch(InsertPt)) {
----------------
This bit of code would make a lot more sense in the caller widenGuard - it's only needed by one of two callers - and would require a lot less code as well.  


================
Comment at: test/Transforms/GuardWidening/basic_explicit_guards.ll:5
+
+declare void @llvm.experimental.guard(i1, ...)
+
----------------
This declaration is unused.  Can you fix that by adding at least two tests mixing guards and widenable conditions?  (i.e. one each widening from the other)


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

https://reviews.llvm.org/D56075