[PATCH] IndVarSimplify: Don't let LFTR compare against a poison value

David Majnemer david.majnemer at gmail.com
Wed Sep 3 17:12:24 PDT 2014


>>! In D5174#13, @atrick wrote:
> I'll say LGTM because fixing a bug is an improvement. This is unfortunate though because LFTR does not care about overflow and we don't want to pessimize code just because SCEV was able to infer NSW. The problem is that we can't express that NSW applies to some uses an not others.

I agree, but using the induction variable seemed better than introducing another addition; I don't know of any machinery in LLVM that will merge a NSW "add" and a normal "add" into a single, normal, add.

> 
> The fix is also not totally robust because SCEV could, in theory, drop NSW flags even though they exist in IR. The induction variable was not necessarily produced by SCEVExpander. I think making it totally robust would involve getting a new expression for CmpIndVar by applying non-NSW AddExpr's (similar to genLoopLimit). That is of course much more complexity, hence risk, so maybe best as a FIXME.

I'll add a FIXME.

> 
> Out of curiosity, can you explain how we came to generate bad code for this case (so I don't have to debug the test case)? Was there an optimization after LFTR that assumed NSW at the compare? Do we end up widening the loop test and failing to exit?

I think we inferred that the rewritten exit condition was unreachable because of the NUW/NSW behavior of the SCEV.

Before my patch, we had BBs that looked like this:
  for.inc13.i:
    %indvars.iv.next.i = add nuw nsw i32 %indvars.iv.i, 1
    br i1 true, label %for.cond2.preheader.i, label %fn1.exit.us-lcssa.us-lcssa

After my patch, the same BB looks like this:
  for.inc13.1:
    %indvars.iv.next.i = add nuw nsw i32 %indvars.iv.i, 1
    br i1 %exitcond19.i, label %for.cond2.preheader.i, label %fn1.exit.us-lcssa.us-lcssa

> 
> FWIW: The check against FlagAnyWrap was a little confusing to me, as opposed to simply:
> if (maskFlags(Expr->getNoWrapFlags(), SCEV::NSW | SCEV::NUW))

I'll clean this up as well.

REPOSITORY
  rL LLVM

http://reviews.llvm.org/D5174






More information about the llvm-commits mailing list