[PATCH] D77560: [SCEV] don't try to query getSCEV() for incomplete PHIs
ChenZheng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 29 07:47:04 PDT 2020
shchenz added a comment.
In D77560#2181768 <https://reviews.llvm.org/D77560#2181768>, @lebedev.ri wrote:
> Somewhat better test (although needs further manual cleanup), that produces different results with/without the fix:
>
> ; RUN: opt %s -loop-reduce -S | FileCheck %s
>
> target datalayout = "e-m:e-i64:64-n32:64"
> target triple = "powerpc64le-unknown-linux-gnu"
>
> %0 = type <{ float }>
>
> define void @foo([0 x %0]* %arg) {
> bb:
> %i = getelementptr inbounds [0 x %0], [0 x %0]* %arg, i64 0, i64 -1
> %i1 = bitcast %0* %i to i8*
> %i2 = getelementptr i8, i8* %i1, i64 4
> br label %bb3
>
> bb3: ; preds = %bb18, %bb
> %i4 = phi i64 [ %i20, %bb18 ], [ 0, %bb ]
> %i5 = phi i64 [ %i21, %bb18 ], [ 1, %bb ]
> br label %bb6
>
> bb6: ; preds = %bb3
> br i1 undef, label %bb7, label %bb8
>
> bb7: ; preds = %bb17, %bb6
> br label %bb22
>
> bb8: ; preds = %bb6
> br label %bb9
>
> bb9: ; preds = %bb9, %bb8
> %i10 = phi i64 [ 0, %bb8 ], [ %i16, %bb9 ]
> %i11 = add i64 %i10, %i4
> %i12 = shl i64 %i11, 2
> %i13 = getelementptr i8, i8* %i2, i64 %i12
> %i14 = bitcast i8* %i13 to float*
> %i15 = bitcast float* %i14 to <4 x float>*
> store <4 x float> undef, <4 x float>* %i15, align 4
> %i16 = add i64 %i10, 32
> br i1 true, label %bb17, label %bb9
>
> bb17: ; preds = %bb9
> br i1 undef, label %bb18, label %bb7
>
> bb18: ; preds = %bb17
> %i19 = add i64 undef, %i4
> %i20 = add i64 %i19, %i5
> %i21 = add nuw nsw i64 %i5, 1
> br label %bb3
>
> bb22: ; preds = %bb22, %bb7
> %i23 = phi i64 [ %i26, %bb22 ], [ undef, %bb7 ]
> %i24 = add nsw i64 %i23, %i4
> %i25 = getelementptr %0, %0* %i, i64 %i24, i32 0
> store float undef, float* %i25, align 4
> %i26 = add nuw nsw i64 %i23, 1
> br label %bb22
> }
>
> I would think `getSCEV()` should be returning `nullptr` if it is asked about an instruction that is currently being created, but i don't know.
Thanks very much for the case. I will update it later. @lebedev.ri
For the solution, the first try for this issue is to return `SCEVUnknown` for an incomplete PHI before this PHI get simplified. So different incomplete PHI get different `SCEVUnknown` objects. I think this is a little like your propose about returning `nullptr`, right?
I think there are several issues for this case:
1: `SCEVExpander` should not call `getSCEV` for incomplete PHIs. We can never reuse an incomplete PHI. This is our current fix.
2: if the instructions are still being built, `getSCEV()` should not simplify them and then return a `SCEVUnknown` object. Different incomplete instructions may have the same simplifying result and then will get same SCEV expression. But after finishing building the instructions, the instructions could be different.
3: When we create a `SCEVUnknown` object for same instruction, should we update the `ExprToIVMap` cache? The first time we create `SCEVUnknown` (say ObjectA) for an incomplete instruction(say InstructionA), we cache them with {ObjectA, InstructionA}. After finishing building the instructions, we may direct call `getUnknown(PHI) ` for InstructionA, this time SCEV will create a new `SCEVUnknown` object, (say `ObjectB`) for the same instruction, but we will not update the `ExprToIVMap` cache. This should be another issue. If you call `getUnknown` many times, you may get different unknown objects, but in the cache `ExprToIVMap`, it is always the legacy one.
Currently we choose the first way to solve this issue per @mkazantsev comments and I think it makes more sense.
But I think issue 2 and issue 3 should still be potential bugs inside scev. But now we don't have cases to expose them.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77560/new/
https://reviews.llvm.org/D77560
More information about the llvm-commits
mailing list