[PATCH] D120586: [Attributor] Add AAAddressSpace to deduce address spaces

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG, assuming the stuff below works out.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12104-12105
+      if (auto *SI = dyn_cast<StoreInst>(Inst))
+        if (SI->getPointerOperand() == AssociatedValue) {
+          MakeChange(Inst, const_cast<Use &>(U));
+          return true;
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > This doesn't actually help as it does not filter `store ptr %p, ptr %p`. Again, if we can just change both uses, then let's do so. If the verifier is unhappy, check that the use `U` is the pointer operand.
> Why does the AS of value operand matter here?
because the verifier might not like us replacing the value `ptr %p` with `ptr addrspace(3) %q`. Make a test with that patter, if it passes, we are A-OK.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12081
+        A.changeUseAfterManifest(U, *OriginalValue);
+      else {
+        Instruction *CastInst = new AddrSpaceCastInst(OriginalValue, NewPtrTy);
----------------
early exit.


================
Comment at: llvm/test/Transforms/Attributor/value-simplify.ll:18
-; CHECK: @[[G:[a-zA-Z0-9_$"\\.-]+]] = internal constant { [2 x ptr] } { [2 x ptr] [ptr @f1, ptr @f2] }
-; CHECK: @[[X:[a-zA-Z0-9_$"\\.-]+]] = external global i32
 ;.
----------------
Can you undo this for now. 


================
Comment at: llvm/test/Transforms/Attributor/value-simplify.ll:804
+; TUNIT-NEXT:    [[TMP1:%.*]] = addrspacecast ptr addrspacecast (ptr addrspace(3) @ConstAS3Ptr to ptr) to ptr addrspace(3)
+; TUNIT-NEXT:    store i32 0, ptr addrspace(3) [[TMP1]], align 4
 ; TUNIT-NEXT:    ret void
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > tianshilei1992 wrote:
> > > This should be replaced by `@ConstAS3Ptr` directly.
> > Did you update the test, above it seems to work.
> I expect it to be `store i32 0, ptr addrspace(3) @ConstAS3Ptr, align 4` directly but actually here it is more complicated than I thought.
Oh, right. Because it is simplified. Leave it for now like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120586