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

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 11 08:32:49 PST 2019

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.



More information about the llvm-commits mailing list