[PATCH] D122268: Add PointerType analysis for DirectX backend

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:61
+  Type *RetTy = F.getReturnType();
+  if (RetTy->isOpaquePointerTy()) {
+    RetTy = nullptr;
----------------
nit: prefer early exits


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:96-99
+  for (auto &G : M.globals()) {
+    if (G.getType()->isOpaquePointerTy())
+      Map[&G] = classifyPointerType(&G);
+  }
----------------
Does this handle global aliases?


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:25
+  // then compare the vectors.
+  SmallVector<Type *, 8> Types;
+  for (auto El : Actual) {
----------------
nit: Why 8? If the exact small size is not that important or know, I think it's better to use the default one: `SmallVector<Type *> Types;`


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:29-34
+  std::sort(Types.begin(), Types.end());
+
+  std::sort(Expected.begin(), Expected.end());
+
+  return memcmp(Types.data(), Expected.data(), Types.size() * sizeof(Type *)) ==
+         0;
----------------
Is this the same as `is_permutation(Types, Expected)`?


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:56-57
+  Type *FnTy = FunctionType::get(Type::getInt64Ty(Context), {I8Ptr}, false);
+  llvm::SmallVector<Type *, 2> Ex = {I8Ptr, FnTy};
+  EXPECT_TRUE(CompareResults(Ex, Map));
+}
----------------
I think it might be more helpful to first extract values from the map and then check, something like:
```
EXPECT_THAT(Ex, UnorderedElementsAreArray(make_second_range(Extracted)));
```

(I haven't tried to compile it, you may need to tweak this a bit.)

The main difference is that gmock macros explain what went wrong when check fail, e.g., which array elements were different.


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:61-74
+  StringRef Assembly = R"(
+    define i32 @test(ptr %p) {
+      store i32 0, ptr %p
+      ret i32 0
+    }
+  )";
+
----------------
Consider creating a test class that takes care of assembling input IR, creating a context, and running the analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122268