[PATCH] D112717: [IR] Replace *all* uses of a constant expression by corresponding instruction

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/lower-kernel-lds-constexpr.ll:114
 ;
-  call void undef(i32* getelementptr inbounds ([505 x i32], [505 x i32]* addrspacecast ([505 x i32] addrspace(3)* @lds.4 to [505 x i32]*), i64 0, i64 0), i32* getelementptr inbounds ([505 x i32], [505 x i32]* addrspacecast ([505 x i32] addrspace(3)* @lds.4 to [505 x i32]*), i64 0, i64 0))
+  call void undef(i32 addrspace(4)* addrspacecast (i32* getelementptr inbounds ([505 x i32], [505 x i32]* addrspacecast ([505 x i32] addrspace(3)* @lds.4 to [505 x i32]*), i64 0, i64 0) to i32 addrspace(4)*),                  i32 addrspace(5)* addrspacecast (i32* getelementptr inbounds ([505 x i32], [505 x i32]* addrspacecast ([505 x i32] addrspace(3)* @lds.4 to [505 x i32]*), i64 0, i64 0) to i32 addrspace(5)*))
   ret void
----------------
arsenm wrote:
> I don't understand this test change. You've introduced an illegal pair of addrspacecasts, going from addrspace(3) to addrspace(0) back to addrspace(4)
Let me first explain in detail, what bug this patch fixes, and then come back to changes within this test.

Consider following Instruction:

```
                                   I
                                 /  \
                               C1   C2
                              /        \
                             C3        C3
```
Let us assume that C3 uses an lds, then we want to convert C3 to an instruction, which in turn forces the paths (C1, C3) and (C2, C3) to convert into instructions.

So, the expected instruction sequence after this constant expression to instruction conversion is as follows by assuming that the path (C1, C2) is processed first, and then the path (C2, C3).

```
I3          (correspond to C3)
I1 (I3)     (correspond to C1)
I2 (I3)     (correspond to C2)
I  (I1, I2)
```

However, before this patch, there was a bug, and due to this bug, the instruction sequence was used to be as below.

```
I3          (correspond to C3) 
I1 (I3)     (correspond to C1)
I2 (C3)     (correspond to C2)
I  (I1, I2)
```

Notice the difference between correct expected result and buggy result. where buggy result gives I2 (C3)  instead of I2 (I3).   This patch fixes this issue.

Now coming back to testcase, I have made changes to this testcase by introducing addrspace constant expressions in order to mimic the above explained scenario, and to test that it is properly handled. Here, I really did not care about legal/illegal aspects of addrspacecasts and also I did not bother about the semantics of the constant expression tree.  

Since it is a compile time testcase and since we care about only code transformation here, I assumed  that it is fine as long as we test for a correct sequence of code transformation after the pass for a given input testcase even though input testcase is illegal from the perspective of actual programming semantics.

However, if you think that we should come-up with a more meaningful testcase here, I will do so.





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112717/new/

https://reviews.llvm.org/D112717