[PATCH] D122268: Add PointerType analysis for DirectX backend

[Chris Bieneman via Phabricator via llvm-commits]

beanz added a comment.

Comments below. Updated patch coming shortly.



================
Comment at: llvm/lib/IR/AsmWriter.cpp:616
+  case Type::DXILPointerTyID:
+    OS << "dxil-ptr (" << Ty << ")";
+    return;
----------------
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.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:42
+      if (NewPointeeTy->isOpaquePointerTy())
+        return TypedPointerType::get(classifyPointerType(User),
+                                     V->getType()->getPointerAddressSpace());
----------------
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.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:102
+
+    for (auto &B : F) {
+      for (auto &I : B) {
----------------
kuhar wrote:
> I find it a bit confusing that we have a helper function `handleFunction` yet still have to iterate over the function instructions here. Can you add a comment describing what `handleFunction` does? Or alternatively, could we move this for loop into `handleFunction`?
The poorly named `handleFunction` helper just generates the function type. The loop for instructions inside only runs if the return type is an opaque pointer to collect the possible return types. I'll add some comments explaining and rename the function more appropriately.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:96-99
+  for (auto &G : M.globals()) {
+    if (G.getType()->isOpaquePointerTy())
+      Map[&G] = classifyPointerType(&G);
+  }
----------------
kuhar wrote:
> kuhar wrote:
> > Does this handle global aliases?
> ping
Not yet. I'm not yet handling any globals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122268