[PATCH] D77560: [SCEV] don't try to query getSCEV() for incomplete PHIs

[ChenZheng via Phabricator via llvm-commits]

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