[PATCH] D27283: Fix invalid addrspacecast due to combining alloca with global var

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:256
+    if (!Inst)
+      return;
+    DEBUG(dbgs() << "Found pointer user: " << *U << '\n');
----------------
rivanvx wrote:
> If the first user isn't an instruction, what guarantees that others aren't? How come there isn't a continue here instead?
The function is recursive. It tries to iterate through the users and then the users of users, so long so forth, to find a chain of GEP/casts that ends with a load, then try to replace the uses of the load with the load of another pointer. The replacing only happens when we reach the load instruction.

For example, we want to replace a with aa, which are pointers to the same pointee type in different addr space:

```
b = bitcast a
c = GEP b, 1
d = load c
// uses of d
```

so we create new casts/GEPs/loads

```
bb = bitcast aa
cc = GEP bb, 1
dd = load cc
// replace uses of d with dd
```

If the first user is not an instruction, that means this will not end up as a chain of instructions, so we will just return so that the recursive function will try the next path.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:287-288
+    auto *NewI = new LoadInst(V);
+    NewI->takeName(LT);
+    IC.InsertNewInstWith(NewI, *LT);
+    IC.replaceInstUsesWith(*LT, NewI);
----------------
rivanvx wrote:
> Does the order matter here? If not, can you make it consistent with other two cases?
The handling of load instructions is different from the handling of GEP/casts. For example, we want to replace a with aa

```
b = cast a
// uses of b
c = load b
// uses of c
```
We will create new casts following the uses of a,


```
bb = cast aa
cc = load bb
// replaces uses of c with uses of cc
```
However, we should not replace uses of b with bb since they have different address space therefore different type. Also we may not be able to remove b since it may have other users. However, since one inst combine pass contains multiple iterations, it will be removed in the next iteration if it does not have any users.



https://reviews.llvm.org/D27283