[PATCH] D103465: [OpaquePtr] Track pointee types in Clang

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
Comment at: clang/lib/CodeGen/Address.h:26
   llvm::Value *Pointer;
+  llvm::Type *PointeeType;
   CharUnits Alignment;
----------------
erichkeane wrote:
> I think this will still end up being a problem when we try to look into the type for multi-dimension pointers.  Should we have this just store the `QualType` of the value instead, so we can unpack it later?  If we ever needed the `llvm::Type` of the Pointee, a `convertTypeForMem` is all that is standing in our way.
Do we do that? I didn't think Address was used that way.


================
Comment at: clang/lib/CodeGen/Address.h:29-30
 public:
   Address(llvm::Value *pointer, CharUnits alignment)
-      : Pointer(pointer), Alignment(alignment) {
+      : Address(pointer, nullptr, alignment) {}
+  Address(llvm::Value *pointer, llvm::Type *PointeeType, CharUnits alignment)
----------------
nikic wrote:
> dblaikie wrote:
> > At some point will this include an assertion that 'pointer' isn't a PointerType? I guess some uses of PointerTyped values won't need to know their pointee type?
> > 
> > (or are all values in Address PointerTyped? (owing to them being "addresses"))?
> Based on the unconditional cast in getType(), I'd assume that addresses are always pointer typed.
This constructor seems odd. We shouldn't ever construct Address with a non-null pointer and a null PointeeType, should we?


================
Comment at: clang/lib/CodeGen/Address.h:31
+      : Address(pointer, nullptr, alignment) {}
+  Address(llvm::Value *pointer, llvm::Type *PointeeType, CharUnits alignment)
+      : Pointer(pointer), PointeeType(PointeeType), Alignment(alignment) {
----------------
craig.topper wrote:
> Is PointeeType expected to be non-null when pointer is non-null?
That would be my expectation.


================
Comment at: clang/lib/CodeGen/Address.h:58
   /// store it in Address instead for the convenience of writing code.
-  llvm::Type *getElementType() const {
-    return getType()->getElementType();
-  }
+  llvm::Type *getElementType() const { return PointeeType; }
 
----------------
craig.topper wrote:
> Should this assert isValid() since it no longer goes through getType() and getPointer() which would have asserted previously?
+1.


================
Comment at: clang/lib/CodeGen/CGValue.h:230-231
 private:
   void Initialize(QualType Type, Qualifiers Quals, CharUnits Alignment,
                   LValueBaseInfo BaseInfo, TBAAAccessInfo TBAAInfo) {
     assert((!Alignment.isZero() || Type->isIncompleteType()) &&
----------------
dblaikie wrote:
> Maybe this could use some assertions added to check the PointeeType is correct?
I think Initialize should be modified to take Addr instead of Alignment, and move
```
    R.V = Addr.getPointer();
    R.PointeeType = Addr.getElementType();
```
into this function, from all the callers.

If that's done, I don't think a separate assert is useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103465