Differential D5174
IndVarSimplify: Don't let LFTR compare against a poison value Authored by majnemer on Sep 3 2014, 10:02 AM.
Details
LinearFunctionTestReplace tries to use the *next* indvar to compare This fixes PR20680.
Diff Detail Event Timeline
Comment Actions We talked about this on IRC; taking the flags from SCEV seems reasonable because SCEV is the one that generated the instruction in the first place (although a comment should be added to that effect). LGTM. Comment Actions 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. 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. 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? FWIW: The check against FlagAnyWrap was a little confusing to me, as opposed to simply: Comment Actions 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.
I'll add a FIXME.
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
I'll clean this up as well. Comment Actions Hi all, We've been seeing some performance regressions in our internal AArch64 tests due to this patch. I'll preface this by saying that I'm not an expert in this matter at all so take everything I say with a grain of salt :) I've reduced the test case (this one is from LNT) to what's attached ( ). Cleaned up, the IR looks something like this:for.cond1.preheader: ; preds = %middle.block, %entry
%indvars.iv17 = phi i64 [ 0, %entry ], [ %indvars.iv.next18, %middle.block ]
%indvars.iv.next18 = add nuw nsw i64 %indvars.iv17, 1
...
middle.block: ; preds = %vector.body
%exitcond19 = icmp eq i64 %indvars.iv17, 255
br i1 %exitcond19, label %for.end10, label %for.cond1.preheaderUsing -target aarch64-linux-gnu -mcpu=cortex-a57 -O3, this compiles to the following assembly, also shortened: .LBB0_1: // %for.cond1.preheader
// =>This Loop Header: Depth=1
// Child Loop BB0_2 Depth 2
add x11, x10, #1 // =1
...
// BB#3: // %middle.block
// in Loop: Header=BB0_1 Depth=1
cmp x10, #255 // =255
mov x10, x11
b.ne .LBB0_1It seems like the exact problem the patch addresses occurs: since the calculation for the next indvar has NUW/NSW flags set, it can't be used to check the exit condition. This leads to an extra register being used for the induction variable. Am I right in thinking that the behaviour in this specific test case is too conservative as a result of the patch? (An i64 won't overflow, since we know the trip count is only 256.) If yes, do you have any suggestions on how we could fix this? I'm assuming this might also affect other targets, at least similar ones such as ARM/AArch32 or Thumb. Any input would be welcome. Cheers Comment Actions What you've done is the right idea. We want to allow post-increment comparison whenever we can prove no overflow on the increment. Handling the constant case as you've done seems safe, albeit messy. A more general way to check overflow is something like this (I haven't tested this in your case): unsigned BitWidth = SE->getTypeSizeInBits(BECount->getType());
// BitWidt+1 should actually be sufficient
Type *WideTy = IntegerType::get(SE->getContext(), BitWidth * 2);
IVCount = SE->getAddExpr(BackedgeTakenCount,
SE->getConstant(BackedgeTakenCount->getType(), 1));
IVCountWide = SE->getAddExpr(
SE->getSignExtendedExpr(BackedgeTakenCount, WideTy),
SE->getConstant(WideTy, 1));
if (getSignExtendExpr(IVCount, WideTy) == IVCountWide)
// no signed overflowYou may want to give this a try and make sure it's still safe in the problem case. | ||||||||||||||||
I'm not 100% sure this is right. I think that IncrementedIndvarSCEV->getNoWrapFlags() will return only the deduced flags (the ones that SCEV can independently prove), and that could be different from the set of flags on the instruction itself. It might be better to test the flags on IncrementedIndvar itself (but, then again, I could just be wrong).