[PATCH] D122268: Add PointerType analysis for DirectX backend

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/include/llvm/IR/Type.h:78
+    ScalableVectorTyID, ///< Scalable SIMD vector type
+    DXILPointerTyID,    ///< DXIL typed pointer used by DirectX target
   };
----------------
nikic wrote:
> Hrm, it's unfortunate that this need to be exposed here.
Sadly yes. That could be changed, but since we do this for X86 types too it is the current pattern.


================
Comment at: llvm/lib/Target/DirectX/DXILPointerType.cpp:35
+  if (!TargetData.hasValue()) {
+    TargetData = Any{std::make_shared<TypedPointerTracking>()};
+  }
----------------
nikic wrote:
> Why does this need shared_ptr?
> 
> nit: Drop braces for single-line if.
llvm::Any (and std::any) data types must be copyable, and it returns copies, not the original object. Since I don't want the underlying DenseMaps to be copied around a million times, needs to be a pointer, and unique_ptr isn't copyable.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:33
+    Type *NewPointeeTy = nullptr;
+    if (auto *Inst = dyn_cast<LoadInst>(User)) {
+      NewPointeeTy = Inst->getType();
----------------
nikic wrote:
> Are instructions like atomicrmw or cmpxchg relevant here?
We don't support those instructions in DXIL. We're not yet handling them in DXILPrepare either, but they'll translate into intrinsics, and we'll need to expand type analysis to cover our intrinsics as they get added.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:43
+        return TypedPointerType::get(classifyPointerType(User),
+                                     V->getType()->getPointerAddressSpace());
+      if (!PointeeTy)
----------------
nikic wrote:
> Why does this one return directly rather than assigning NewPointeeTy?
It is returning with a recursive call in the argument. This follows LLVM convention for early returns since the subsequent code isn't applicable.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:89
+    return;
+  Map[&F] = FunctionType::get(RetTy, NewArgs, false);
+  ;
----------------
nikic wrote:
> Does not preserve variadic function type?
We don't support variadic functions in the source language, so they shouldn't be showing up in the IR. I can add an assert to hard fail that case.


================
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;
----------------
nikic wrote:
> But this doesn't check that the type is actually assigned to the correct value, right? Just that the type is present somewhere?
That's a fair observation. I can update the test to be more robust.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122268