[PATCH] D23749: [NVPTX] Add NVPTXHoistAddrSpaceCast pass.

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: llvm/lib/Target/NVPTX/NVPTXHoistAddrSpaceCast.cpp:83-87
@@ +82,7 @@
+//
+// You can think of hoisting an addrspacecast as propagating up the CFG an
+// assertion that a particular pointer lives in a particular address space.
+// It follows that we can only hoist an addrspacecast above an instruction if
+// the cast postdominates the instruction -- otherwise, our assertion might be
+// unfounded!
+//
----------------
Hmm, this looks a little suspicious to me.

What if you have:
    %gep = ...
    %call = call void @never_returns(%gep)
    %asc = addrspacecast %gep ...

Just because the addrspacecast postdominates the GEP does not ensure that it will be executed.

Along slightly similar but slightly different lines, consider:
    %gep = ...
    %asc = addrspacecast %gep ...
    br i1 %cond, %then, %else

  true:
    [... never use %asc, only use %gep ...]

  false:
    [... use %asc ...]

In this case the addrspacecast postdominates the GEP and all its uses are control dependent on `%cond`.  If `%cond` is true, then the addrspacecast is never used and all access to memory go through `%gep` instead of `%asc`.

================
Comment at: llvm/lib/Target/NVPTX/NVPTXHoistAddrSpaceCast.cpp:155
@@ +154,3 @@
+  for (Instruction &I : instructions(F))
+    if (AddrSpaceCastInst *ASC = dyn_cast<AddrSpaceCastInst>(&I))
+      if (ASC->getSrcAddressSpace() == AddressSpace::ADDRESS_SPACE_GENERIC &&
----------------
`auto *`

================
Comment at: llvm/lib/Target/NVPTX/NVPTXHoistAddrSpaceCast.cpp:168
@@ +167,3 @@
+    // We currently only hoist addrspacecasts above GEPs.
+    GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(ASC->getOperand(0));
+    if (!GEP)
----------------
Ditto.

================
Comment at: llvm/lib/Target/NVPTX/NVPTXHoistAddrSpaceCast.cpp:202-204
@@ +201,5 @@
+    Value *GEPOp = GEP->getOperand(0);
+    AddrSpaceCastInst *NewASC =
+        new AddrSpaceCastInst(GEPOp, ASC->getType(), ASC->getName(), GEP);
+    GetElementPtrInst *NewGEP = GetElementPtrInst::Create(
+        GEP->getSourceElementType(), NewASC,
----------------
Ditto.

================
Comment at: llvm/lib/Target/NVPTX/NVPTXHoistAddrSpaceCast.cpp:210-211
@@ +209,4 @@
+
+    AddrSpaceCastInst *NewASCToGeneric =
+        new AddrSpaceCastInst(NewGEP, GEP->getType(), ASC->getName(), GEP);
+
----------------
Ditto.


https://reviews.llvm.org/D23749