[CodeGenPrepare] Don't sink non-cheap addrspacecasts.

[jlebar]

Summary

Previously, CGP would unconditionally sink addrspacecast instructions,
even going so far as to sink them into a loop.

Now we check that the cast is "cheap", as defined by TLI.

We introduce a new "is-cheap" function to TLI rather than using
isNopAddrSpaceCast because some GPU platforms want the ability to ask
for non-nop casts to be sunk.

[tra]

llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll

15

Could that be changed for a positive check at the location where you *do* want to see that addrspacecast?

[jlebar]

llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll

15

What problem would this solve?

[tra]

llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll

15

Not having addrspacecast here does not prove that it's where it should be.
I don't know what exactly codegenprepare does, but what if sinking of addrspacecast did happen and for whatever reason it created its own BB just ahead of l2? The test would pass. This scenario is may be too far-fetched, so I'm OK with the test either way.

[jlebar]

Update test.

llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll

15

No, I think that's reasonable. I added explicit checks that I think should catch this case, thanks.

@arsenm, I would still like your review on this, since you're the impetus for isCheapAddrSpaceCast as opposed to isNoopAddrSpaceCast.

[arsenm]

LGTM

llvm/lib/CodeGen/CodeGenPrepare.cpp

829–832

Braces

[jlebar]

llvm/lib/CodeGen/CodeGenPrepare.cpp

829–832

I thought we always omitted them if legal?

(I am very happy to adopt a different rule; I don't like this one at all, fwiw.)

llvm/include/llvm/Target/TargetLowering.h

public:
   /// Returns the target-specific address of the unsafe stack pointer.
   virtual Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const;
 
   /// Returns true if a cast between SrcAS and DestAS is a noop.
   virtual bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const {
     return false;
   }
 
+  /// Returns true if a cast from SrcAS to DestAS is "cheap", such that e.g. we
+  /// are happy to sink it into basic blocks.
+  virtual bool isCheapAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const {
+    return isNoopAddrSpaceCast(SrcAS, DestAS);
+  }
+
   /// Return true if the pointer arguments to CI should be aligned by aligning
   /// the object whose address is being passed. If so then MinSize is set to the
   /// minimum size the object must be to be aligned and PrefAlign is set to the
   /// preferred alignment.
   virtual bool shouldAlignPointerArgs(CallInst * /*CI*/, unsigned & /*MinSize*/,
                                       unsigned & /*PrefAlign*/) const {
     return false;
   }

llvm/lib/CodeGen/CodeGenPrepare.cpp

 /// If the specified cast instruction is a noop copy (e.g. it's casting from
 /// one pointer type to another, i32->i8 on PPC), sink it into user blocks to
 /// reduce the number of virtual registers that must be created and coalesced.
 ///
 /// Return true if any changes are made.
 ///
 static bool OptimizeNoopCopyExpression(CastInst *CI, const TargetLowering &TLI,
                                        const DataLayout &DL) {
+  // Sink only "cheap" (or nop) address-space casts.  This is a weaker condition
+  // than sinking only nop casts, but is helpful on some platforms.
+  if (auto *ASC = dyn_cast<AddrSpaceCastInst>(CI))
+    if (!TLI.isCheapAddrSpaceCast(ASC->getSrcAddressSpace(),
+                                  ASC->getDestAddressSpace()))
+      return false;
+
   // If this is a noop copy,
   EVT SrcVT = TLI.getValueType(DL, CI->getOperand(0)->getType());
   EVT DstVT = TLI.getValueType(DL, CI->getType());
 
   // This is an fp<->int conversion?
   if (SrcVT.isInteger() != DstVT.isInteger())
     return false;
 

llvm/test/Transforms/CodeGenPrepare/NVPTX/dont-sink-nop-addrspacecast.ll

+; RUN: opt -S -codegenprepare < %s | FileCheck %s
+
+target datalayout = "e-i64:64-v16:16-v32:32-n16:32:64"
+target triple = "nvptx64-nvidia-cuda"
+
+; CHECK-LABEL: @test
+define i64 @test(i1 %pred, i64* %ptr) {
+; CHECK: addrspacecast
+  %ptr_as1 = addrspacecast i64* %ptr to i64 addrspace(1)*
+  br i1 %pred, label %l1, label %l2
+l1:
+; CHECK-LABEL: l1:
+; CHECK-NOT: addrspacecast
+  %v1 = load i64, i64* %ptr
+  ret i64 %v1
+l2:
+  ; CHECK-LABEL: l2:
+  ; CHECK-NOT: addrspacecast
+  %v2 = load i64, i64 addrspace(1)* %ptr_as1
+  ret i64 %v2
+}