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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9910
+    return getAssumed();
+  }
+
----------------
hasXXX sounds like a boolean return, this would be getAS


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9917
+
+    A.getAAFor<AAIsDead>(*this, getIRPosition(), DepClassTy::REQUIRED);
+  }
----------------
don't make it required, make the dependence type NONE


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9924
+    AA::getAssumedUnderlyingObjects(A, getAssociatedValue(), Objects, *this,
+                                    getCtxI(), UsedAssumedInformation);
+
----------------
Check the return type of this function.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9931
+      if (!V || !V->getType()->isPointerTy())
+        continue;
+
----------------
That doesn't work. We cannot ignore things with the "wrong type".
V should never be null, undef can be ignored, a "zero" too if nullptr is not valid, everything else not I think.
See some other uses of getAssumedUnderlyingObjects.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9938
+      else if (AddressSpace != NewAddressSpace)
+        AddressSpace = getWorstState();
+    }
----------------
break


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9957
+    Instruction *Cast = new AddrSpaceCastInst(AssociatedValue, NewPtrTy);
+    Cast->insertAfter(CtxI);
+
----------------
before, not after.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9982
+      }
+    }
+
----------------
We should reuse the rewrite capabilities of the existing pass, or at least clone loads/stores properly so we don't loose metadata and such.


================
Comment at: llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll:233
 ; IS__CGSCC_NPM-NEXT:    [[TMP4:%.*]] = getelementptr [[STRUCT_MYSTR]], %struct.MYstr* [[U_PRIV]], i32 0, i32 0
-; IS__CGSCC_NPM-NEXT:    [[TMP5:%.*]] = load i8, i8* [[TMP4]], align 8
+; IS__CGSCC_NPM-NEXT:    [[TMP5:%.*]] = load i8, i8* undef, align 8
 ; IS__CGSCC_NPM-NEXT:    [[TMP6:%.*]] = zext i8 0 to i32
----------------
This indicates a problem, though, not necessarily in your code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120586