[PATCH] D122268: Add PointerType analysis for DirectX backend

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/IR/LLVMContext.h:321
+  /// management and bypassing layering restrictions.
+  llvm::Any &getTargetData() const;
+
----------------
The addition of this API should be separated into a separate review (with a unit test).


================
Comment at: llvm/include/llvm/IR/Type.h:78
+    ScalableVectorTyID, ///< Scalable SIMD vector type
+    DXILPointerTyID,    ///< DXIL typed pointer used by DirectX target
   };
----------------
Hrm, it's unfortunate that this need to be exposed here.


================
Comment at: llvm/lib/Target/DirectX/DXILPointerType.cpp:35
+  if (!TargetData.hasValue()) {
+    TargetData = Any{std::make_shared<TypedPointerTracking>()};
+  }
----------------
Why does this need shared_ptr?

nit: Drop braces for single-line if.


================
Comment at: llvm/lib/Target/DirectX/DXILPointerType.h:2
+//===- Target/DirectX/DXILTypedPointerType.h - DXIL Typed Pointer Type
+//---------===//
+//
----------------
File name out of sync, also in the header guard.


================
Comment at: llvm/lib/Target/DirectX/DXILPointerType.h:46
+  /// pointer transition.
+  /// TODO: remove after opaque pointer transition is complete.
+  static TypedPointerType *getWithSamePointeeType(TypedPointerType *PT,
----------------
Do you actually need these helpers? The TODO certainly doesn't make sense...


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:33
+    Type *NewPointeeTy = nullptr;
+    if (auto *Inst = dyn_cast<LoadInst>(User)) {
+      NewPointeeTy = Inst->getType();
----------------
Are instructions like atomicrmw or cmpxchg relevant here?


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:43
+        return TypedPointerType::get(classifyPointerType(User),
+                                     V->getType()->getPointerAddressSpace());
+      if (!PointeeTy)
----------------
Why does this one return directly rather than assigning NewPointeeTy?


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:57
+
+void handleFunction(const llvm::Function &F, PointerTypeMap &Map) {
+  SmallVector<Type *, 8> NewArgs;
----------------
Unnecessary llvm prefix.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:89
+    return;
+  Map[&F] = FunctionType::get(RetTy, NewArgs, false);
+  ;
----------------
Does not preserve variadic function type?


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:90
+  Map[&F] = FunctionType::get(RetTy, NewArgs, false);
+  ;
+}
----------------
Spurious semicolon.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.h:16
+
+#include "DXILPointerType.h"
+#include "llvm/ADT/DenseMap.h"
----------------
Include doesn't look necessary.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.h:30
+/// This code relies on typed pointers existing as LLVM types, but could be
+/// migrated to a custom Type if PointerType loses typed support.
+namespace PointerTypeAnalysis {
----------------
Last two lines are outdated.


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:20
+
+bool CompareResults(llvm::SmallVectorImpl<Type *> &Expected,
+                    PointerTypeMap &Actual) {
----------------
`static` and unnecessary llvm prefix.


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:24
+  // be, and they should match pointers. So I throw them into vectors, sort them
+  // then compare the vectors.
+  SmallVector<Type *, 8> Types;
----------------
But this doesn't check that the type is actually assigned to the correct value, right? Just that the type is present somewhere?


================
Comment at: llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp:46
+
+  // Parse the IR. The two calls in @test can not access aliasing elements.
+  LLVMContext Context;
----------------
Comment doesn't make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122268