In r337830 I added SCEV checks to enable us to insert fewer bounds checks. Unfortunately, this sometimes crashes when multiple bounds checks are added due to SCEV caching issues. This patch splits the bounds checking pass into two phases, one that computes all the conditions (using SCEV checks) and the other that adds the new instructions.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Transforms/Instrumentation/BoundsChecking.cpp | ||
---|---|---|
162 ↗ | (On Diff #157794) | Move GetTrapBB definition near the end of the function, where it is first used. |
184 ↗ | (On Diff #157794) | It's very uncommon to store builders in containers. Replace with an instruction pointer. TrapInfo can be combined with WorkList, and both loops can be merged in one. |
I found another reproducer for the problem. This one is slightly different where a different assert blow, but I guess its only a different symptom for the same fault. As this one is slightly smaller you might want to use this one as a testcase instead (or use both)? Run with "opt -bounds-checking -S".
@d = dso_local local_unnamed_addr global [1 x i16] zeroinitializer, align 1
@e = dso_local local_unnamed_addr global [1 x i16] zeroinitializer, align 1
define dso_local void @test2() local_unnamed_addr {
entry:
br label %while.cond1.preheader
while.cond1.preheader:
%0 = phi i16 [ undef, %entry ], [ %inc, %while.end ] %1 = load i16, i16* undef, align 1 br label %while.end
while.end:
%inc = add nsw i16 %0, 1 %arrayidx = getelementptr inbounds [1 x i16], [1 x i16]* @e, i16 0, i16 %0 %2 = load i16, i16* %arrayidx, align 1 br i1 false, label %while.end6, label %while.cond1.preheader
while.end6:
ret void
}
Thanks! I'll use both it and the first one you gave me. Luckily, it doesn't crash with this patch.
lib/Transforms/Instrumentation/BoundsChecking.cpp | ||
---|---|---|
184 ↗ | (On Diff #157794) | How did you want me to combine TrapInfo with WorkList? To insert the traps, I need both the original instruction (to create the builder) and the newly-created Or. I could change Worklist to hold a tuple with an optional Or, but that seems more complicated than using two lists like this. |
lib/Transforms/Instrumentation/BoundsChecking.cpp | ||
---|---|---|
159 ↗ | (On Diff #157976) | The loop over worklist can be replaced with a loop over instructions(F). It would work because we no longer create new basic blocks in the loop, and new instructions are added before the current iterator position, so they will not be observed in the following iterations. |
lib/Transforms/Instrumentation/BoundsChecking.cpp | ||
---|---|---|
159 ↗ | (On Diff #157976) | Ah, you meant combine this loop with the previous one, not the new one I added below? Sorry, I misunderstood. That makes sense. |
Friendly ping. This also fixes https://bugs.llvm.org/show_bug.cgi?id=38438.
LGTM, thanks!
Sorry for the delay, I did not notice that you've updated the patch with your last comment.