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

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/two required follow ups.  Can be before or after landing.



================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:275
+    Value *WidenableCondition = nullptr;
+    if (isGuardAsWidenableBranch(ToWiden)) {
+      auto *Cond = cast<BranchInst>(ToWiden)->getCondition();
----------------
minor: We're not changing toWiden, so this can be done as a single check below the call to widenCondCommon


================
Comment at: test/Transforms/GuardWidening/basic_explicit_guards.ll:5
+
+declare void @llvm.experimental.guard(i1, ...)
+
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > reames wrote:
> > > 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)
> > These tests have already be derived from `test/Transforms/GuardWidening/basic.ll` Does it make sense to duplicate?
> i.e. these tests are directly derived from tests from basic.ll using `MakeGuardsExplicit` pass. I'm not sure that it makes sense to have them in two files at the same time.
Max, I can't follow your response to my comment here.  I'm asking for a test mixing guards and wideable conditions.  That is not tested elsewhere.  I don't care which file you put the tests in provided you add them.


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

https://reviews.llvm.org/D56075