[PATCH] D122268: Add PointerType analysis for DirectX backend

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/IR/AsmWriter.cpp:616
+  case Type::DXILPointerTyID:
+    OS << "dxil-ptr (" << Ty << ")";
+    return;
----------------
This prints `dixl-ptr (0x...)` instead of the element type. Is this intentional? Could we have a test for this?


================
Comment at: llvm/lib/Target/DirectX/DXILPointerType.h:39
+  /// Return the address space of the Pointer type.
+  inline unsigned getAddressSpace() const { return getSubclassData(); }
+
----------------
nit: do we need inline here?


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:25
+  if (auto *Inst = dyn_cast<GetElementPtrInst>(V)) {
+    PointeeTy = Inst->getResultElementType()->isOpaquePointerTy()
+                    ? nullptr
----------------
Make this a nested `if` instead. We don't need an `else` because `PointeeTy` is already initialized to `nullptr` above.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:31
+  }
+  for (auto User : V->users()) {
+    Type *NewPointeeTy = nullptr;
----------------
nit: `const auto *User`


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:42
+      if (NewPointeeTy->isOpaquePointerTy())
+        return TypedPointerType::get(classifyPointerType(User),
+                                     V->getType()->getPointerAddressSpace());
----------------
What's the worst case for the number of recursive calls? Could a pathological shader cause a stack overflow?


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:102
+
+    for (auto &B : F) {
+      for (auto &I : B) {
----------------
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`?


================
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:
> Does this handle global aliases?
ping


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:42
+    if (isa<Function>(Entry.first))
+      EXPECT_EQ(Entry.second, FnTy);
+    else if (isa<Argument>(Entry.first))
----------------
IMO it would be more readable/idiomatic to use gmock matchers here, e.g.:

```
#include "gmock/gmock.h"

using ::testing::Contains;
using ::testing::Pair;

template <typename T> struct IsA {
  friend bool operator==(const Value *V, const IsA &) { return isa<T>(V); }
};

// ...
  ASSERT_EQ(Map.size(), 2u);
  Type *I8Ptr = TypedPointerType::get(Type::getInt8Ty(Context), 0);
  Type *FnTy = FunctionType::get(Type::getInt64Ty(Context), {I8Ptr}, false);
  EXPECT_THAT(Map, Contains(Pair(IsA<Function>(), FnTy)));
  EXPECT_THAT(Map, Contains(Pair(IsA<ReturnInst>(), I8Ptr)))
```

Or if you would like something with descriptive failure messages:
```
template <typename T> bool HasType(const Value *V) { return isa<T>(V); }

MATCHER_P2(HasOneEntry, key_predicate, value_type, "") {
  bool Found = false;
  for (auto &KV : arg) {
    if (key_predicate(KV.first)) {
      if (Found) {
        *result_listener << "where duplicate keys were found";
        return false;
      }
      if (KV.second != value_type) {
        *result_listener << "where the key was found but value did not match";
        return false;
      }
      Found = true;
    }
  }

  if (!Found) {
    *result_listener << "where the key is missing";
    return false;
  }
  return true;
}

// ...

  EXPECT_THAT(Map, HasOneEntry(HasType<ReturnInst>, I8Ptr));
```



================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:208
+  }
+}
----------------
Could we have a new test case that deals with globals?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122268