[PATCH] D122269: Modify DXILPrepare to emit no-op bitcasts

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:87-88
     AttributeMask AttrMask;
     for (Attribute::AttrKind I = Attribute::None; I != Attribute::EndAttrKinds;
          I = Attribute::AttrKind(I + 1)) {
       if (!isValidForDXIL(I))
----------------
nit: If you want to revisit this in a future patch, I think you can use `enum_seq` here: https://llvm.org/doxygen/Sequence_8h.html


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:95
       F.removeRetAttrs(AttrMask);
       for (size_t Idx = 0; Idx < F.arg_size(); ++Idx)
         F.removeParamAttrs(Idx, AttrMask);
----------------
I know this is existing code, but a tiny nit: I think the coding style prefers the end value to be calculated only once, e.g., `for (size_t i = 0, e = size(); i != e; ++i) ...`


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:109
+          }
+          // Only insert bitcasts if the IR is using opaque pointers
+          if (!M.getContext().hasSetOpaquePointersValue())
----------------
nit: missing `.` at the end of the comment


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:115
+          // unmodified as it reserves instruction IDs during contruction.
+          if (auto Inst = dyn_cast_or_null<LoadInst>(&I)) {
+            // Omit bitcasts if the incoming value matches the instruction type
----------------
Since `I ` is a reference, it cannot be `null`, so we should use plain `dyn_cast`.
Second, the type on the left should be `auto *`.
Last, could we give it a more descriptive name since we know the type? Maybe `Load` or `LI`.


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:116
+          if (auto Inst = dyn_cast_or_null<LoadInst>(&I)) {
+            // Omit bitcasts if the incoming value matches the instruction type
+            auto It = PointerTypes.find(Inst->getPointerOperand());
----------------
nit: missing `.` at the end of the comment


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:117-118
+            // Omit bitcasts if the incoming value matches the instruction type
+            auto It = PointerTypes.find(Inst->getPointerOperand());
+            if (It != PointerTypes.end()) {
+              if (cast<TypedPointerType>(It->second)->getElementType() ==
----------------
`It` is shadowed by other iterator variables below. Can we give it a more descriptive/unique name?


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:133
+          }
+          if (auto Inst = dyn_cast_or_null<StoreInst>(&I)) {
+            // Omit bitcasts if the incoming value matches the instruction type
----------------
same here


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:134
+          if (auto Inst = dyn_cast_or_null<StoreInst>(&I)) {
+            // Omit bitcasts if the incoming value matches the instruction type
+            auto It = PointerTypes.find(Inst->getPointerOperand());
----------------
also here


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:150
+          }
+          if (auto Inst = dyn_cast_or_null<GetElementPtrInst>(&I)) {
+            // Omit bitcasts if the incoming value matches the instruction type
----------------
same here


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:151
+          if (auto Inst = dyn_cast_or_null<GetElementPtrInst>(&I)) {
+            // Omit bitcasts if the incoming value matches the instruction type
+            auto It = PointerTypes.find(Inst->getPointerOperand());
----------------
also here


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:152-161
+            auto It = PointerTypes.find(Inst->getPointerOperand());
+            if (It != PointerTypes.end()) {
+              if (cast<TypedPointerType>(It->second)->getElementType() ==
+                  Inst->getResultElementType())
+                continue;
+            }
+            Builder.SetInsertPoint(Inst);
----------------
This pattern seems pretty repetitive: for each type, we check if the pointer operand is in the type, adjust the inter point, and emit a bitcast. Could we move it to a generic helper function? Something like:
```
if (Value *Bitcast = createNoopBitcast(Inst, Inst->getPointerOperand(), Inst->getPointerOperandType()) {
  // Update or replace Inst.
  continue;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122269