[PATCH] D122268: Add PointerType analysis for DirectX backend

[Jakub Kuderski via Phabricator via llvm-commits]

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

LGTM modulo nits. Thanks for the fixes!



================
Comment at: llvm/lib/IR/AsmWriter.cpp:616
+  case Type::DXILPointerTyID:
+    OS << "dxil-ptr (" << Ty << ")";
+    return;
----------------
kuhar wrote:
> beanz wrote:
> > kuhar wrote:
> > > This prints `dixl-ptr (0x...)` instead of the element type. Is this intentional? Could we have a test for this?
> > Yea, I did this intentionally so that the `llvm::Type` class doesn't need to preserve interfaces for typed pointers, and so that the impact of DXILPointerType outside the backend is kept at a minimum.
> This is surprising to me, I'd expect types to produce the same strings across multiple runs. Could ignore the addresses altogether, use some stable ID, or add a comment explaining this choice here?
also this


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:21
+
+// classify the type of the value passed in by walking the value's users to find
+// a typed instruction to materialize a type from.
----------------
nit: s/classify/Classifies


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:42
+      if (NewPointeeTy->isOpaquePointerTy())
+        return TypedPointerType::get(classifyPointerType(User),
+                                     V->getType()->getPointerAddressSpace());
----------------
kuhar wrote:
> beanz wrote:
> > kuhar wrote:
> > > What's the worst case for the number of recursive calls? Could a pathological shader cause a stack overflow?
> > Since the source language doesn't support pointers, we should rarely see more than two levels of indirection.
> OK, that makes sense to me. Could you add this as a comment?
also this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122268