[InstCombine] Add an option to disable addrspacecast folding into GEP

[mareko]

Summary

AMDGPU users use addrspacecast to zero-extend a 32-bit pointer to 64 bits.
No other instruction is ever used on 32-bit pointers.

AMDGPU doesn't have full support for 32-bit pointers everywhere, so make
sure addrspacecast isn't folded, so that we don't get GEP on 32-bit
pointers.

[mareko]

FYI, I'd like to get this into LLVM 6.0 if it's OK.

[davide]

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?
Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

[mareko]

In D41652#965765, @davide wrote:

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

[davide]

In D41652#965797, @mareko wrote:

In D41652#965765, @davide wrote:

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

This is quite unfortunate. I'd like to point out I don't feel particularly comfortable to have this as a long term solution (but, maybe, OK for 6.0 & reverted in trunk).
The contract between the mid-level optimizer and the backend is that the latter should possibly accept everything produced by the former (or, FWIW, error out in some circumstances).
Maybe you can have something in your backend logic that recovers from the fact that AMDGPU doesn't know (and won't be taught) about 32-bit GEPs?
Have you considered something during legalization? [Apologies if I'm off, but I'm not really familiar with the way AMDGPU works, at least not in depth].

[mareko]

In D41652#965806, @davide wrote:

In D41652#965797, @mareko wrote:

In D41652#965765, @davide wrote:

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

This is quite unfortunate. I'd like to point out I don't feel particularly comfortable to have this as a long term solution (but, maybe, OK for 6.0 & reverted in trunk).
The contract between the mid-level optimizer and the backend is that the latter should possibly accept everything produced by the former (or, FWIW, error out in some circumstances).
Maybe you can have something in your backend logic that recovers from the fact that AMDGPU doesn't know (and won't be taught) about 32-bit GEPs?
Have you considered something during legalization? [Apologies if I'm off, but I'm not really familiar with the way AMDGPU works, at least not in depth].

GEPs will actually work, but the generated assembly for loads will be worse, thus I'd like to avoid them. A better solution for LLVM 7.0 might be possible.

[davide]

In D41652#965836, @mareko wrote:

In D41652#965806, @davide wrote:

In D41652#965797, @mareko wrote:

In D41652#965765, @davide wrote:

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

This is quite unfortunate. I'd like to point out I don't feel particularly comfortable to have this as a long term solution (but, maybe, OK for 6.0 & reverted in trunk).
The contract between the mid-level optimizer and the backend is that the latter should possibly accept everything produced by the former (or, FWIW, error out in some circumstances).
Maybe you can have something in your backend logic that recovers from the fact that AMDGPU doesn't know (and won't be taught) about 32-bit GEPs?
Have you considered something during legalization? [Apologies if I'm off, but I'm not really familiar with the way AMDGPU works, at least not in depth].

GEPs will actually work, but the generated assembly for loads will be worse, thus I'd like to avoid them. A better solution for LLVM 7.0 might be possible.

Cool. After you update the patch (and we reach consensus), I guess it's not unreachable to commit this to trunk, back port to 6, then revert and start working on a better solution for the 7.0 timeframe.
I do understand we all have deadlines at times :)

[aprantl]

include/llvm/IR/Value.h

512

The \brief is redundant, we use autobrief now.

[mareko]

Abandoning. The new plan is that we'll do it the right way in AMDGPU now.

include/llvm/IR/Value.h

#include "llvm/IR/Value.def"
   /// Returns the original uncasted value.  If this is called on a non-pointer
   /// value, it returns 'this'.
   const Value *stripPointerCasts() const;
   Value *stripPointerCasts() {
     return const_cast<Value *>(
                          static_cast<const Value *>(this)->stripPointerCasts());
   }
 
+  /// \brief Strip off pointer casts (except addrspacecast), all-zero GEPs,
+  /// and aliases.
+  ///
+  /// Returns the original uncasted value or addrspacecast (if present).
+  /// If this is called on a non-pointer value, it returns 'this'.
+  const Value *stripPointerCastsKeepAddrSpaceCast() const;
+  Value *stripPointerCastsKeepAddrSpaceCast() {
+    return const_cast<Value *>(
+      static_cast<const Value *>(this)->stripPointerCastsKeepAddrSpaceCast());
+  }
+
   /// \brief Strip off pointer casts, all-zero GEPs, aliases and barriers.
   ///
   /// Returns the original uncasted value.  If this is called on a non-pointer
   /// value, it returns 'this'. This function should be used only in
   /// Alias analysis.
   const Value *stripPointerCastsAndBarriers() const;
   Value *stripPointerCastsAndBarriers() {
     return const_cast<Value *>(

lib/IR/Value.cpp

for (auto *C : Constants)
     C->handleOperandChange(this, New);
 }
 
 namespace {
 // Various metrics for how much to strip off of pointers.
 enum PointerStripKind {
   PSK_ZeroIndices,
   PSK_ZeroIndicesAndAliases,
+  PSK_ZeroIndicesAndAliasesKeepAddrSpaceCast,
   PSK_ZeroIndicesAndAliasesAndBarriers,
   PSK_InBoundsConstantIndices,
   PSK_InBounds
 };
 
 template <PointerStripKind StripKind>
 static const Value *stripPointerCastsAndOffsets(const Value *V) {
   if (!V->getType()->isPointerTy())
     return V;
 
   // Even though we don't look through PHI nodes, we could be called on an
   // instruction in an unreachable block, which may be on a cycle.
   SmallPtrSet<const Value *, 4> Visited;
 
   Visited.insert(V);
   do {
     if (auto *GEP = dyn_cast<GEPOperator>(V)) {
       switch (StripKind) {
       case PSK_ZeroIndicesAndAliases:
+      case PSK_ZeroIndicesAndAliasesKeepAddrSpaceCast:
       case PSK_ZeroIndicesAndAliasesAndBarriers:
       case PSK_ZeroIndices:
         if (!GEP->hasAllZeroIndices())
           return V;
         break;
       case PSK_InBoundsConstantIndices:
         if (!GEP->hasAllConstantIndices())
           return V;
         LLVM_FALLTHROUGH;
       case PSK_InBounds:
         if (!GEP->isInBounds())
           return V;
         break;
       }
       V = GEP->getPointerOperand();
     } else if (Operator::getOpcode(V) == Instruction::BitCast ||
-               Operator::getOpcode(V) == Instruction::AddrSpaceCast) {
+               (StripKind != PSK_ZeroIndicesAndAliasesKeepAddrSpaceCast &&
+                Operator::getOpcode(V) == Instruction::AddrSpaceCast)) {
       V = cast<Operator>(V)->getOperand(0);
     } else if (auto *GA = dyn_cast<GlobalAlias>(V)) {
       if (StripKind == PSK_ZeroIndices || GA->isInterposable())
         return V;
       V = GA->getAliasee();
     } else {
       if (auto CS = ImmutableCallSite(V)) {
         if (const Value *RV = CS.getReturnedArgOperand()) {
static const Value *stripPointerCastsAndOffsets(const Value *V) {
   return V;
 }
 } // end anonymous namespace
 
 const Value *Value::stripPointerCasts() const {
   return stripPointerCastsAndOffsets<PSK_ZeroIndicesAndAliases>(this);
 }
 
+const Value *Value::stripPointerCastsKeepAddrSpaceCast() const {
+  return stripPointerCastsAndOffsets<
+    PSK_ZeroIndicesAndAliasesKeepAddrSpaceCast>(this);
+}
+
 const Value *Value::stripPointerCastsNoFollowAliases() const {
   return stripPointerCastsAndOffsets<PSK_ZeroIndices>(this);
 }
 
 const Value *Value::stripInBoundsConstantOffsets() const {
   return stripPointerCastsAndOffsets<PSK_InBoundsConstantIndices>(this);
 }
 

lib/Transforms/InstCombine/InstructionCombining.cpp

 // increases variable availability at the cost of accuracy. Variables that
 // cannot be promoted by mem2reg or SROA will be described as living in memory
 // for their entire lifetime. However, passes like DSE and instcombine can
 // delete stores to the alloca, leading to misleading and inaccurate debug
 // information. This flag can be removed when those passes are fixed.
 static cl::opt<unsigned> ShouldLowerDbgDeclare("instcombine-lower-dbg-declare",
                                                cl::Hidden, cl::init(true));
 
+static cl::opt<bool>
+NoAddrSpaceCastFolding("instcombine-no-addrspacecast-folding",
+                       cl::desc("Do not fold addrspacecast into GEP."));
+
 Value *InstCombiner::EmitGEPOffset(User *GEP) {
   return llvm::EmitGEPOffset(&Builder, DL, GEP);
 }
 
 /// Return true if it is desirable to convert an integer computation from a
 /// given bit width to a new bit width.
 /// We don't want to convert from a legal to an illegal type or from a smaller
 /// to a larger illegal type. A width of '1' is always treated as a legal type
if (GEP.getNumIndices() == 1) {
     }
   }
 
   // We do not handle pointer-vector geps here.
   if (GEP.getType()->isVectorTy())
     return nullptr;
 
   // Handle gep(bitcast x) and gep(gep x, 0, 0, 0).
-  Value *StrippedPtr = PtrOp->stripPointerCasts();
+  Value *StrippedPtr;
+  if (NoAddrSpaceCastFolding)
+    StrippedPtr = PtrOp->stripPointerCastsKeepAddrSpaceCast();
+  else
+    StrippedPtr = PtrOp->stripPointerCasts();
   PointerType *StrippedPtrTy = cast<PointerType>(StrippedPtr->getType());
 
   if (StrippedPtr != PtrOp) {
     bool HasZeroPointerIndex = false;
     if (ConstantInt *C = dyn_cast<ConstantInt>(GEP.getOperand(1)))
       HasZeroPointerIndex = C->isZero();
 
     // Transform: GEP (bitcast [10 x i8]* X to [0 x i8]*), i32 0, ...

test/Transforms/InstCombine/cast-no-addrspacecast-folding.ll

+; RUN: opt < %s -instcombine -instcombine-no-addrspacecast-folding=true -S | FileCheck %s
+target datalayout = "E-p:64:64:64-p1:32:32:32-p2:64:64:64-p3:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128-n8:16:32:64"
+
+define double @test80_addrspacecast(double addrspace(1)* %p, i32 %i) {
+; CHECK-LABEL: @test80_addrspacecast(
+; CHECK-NEXT:    [[Q:%.*]] = addrspacecast double addrspace(1)* %p to double addrspace(2)*
+; CHECK-NEXT:    [[S:%.*]] = sext i32 %i to i64
+; CHECK-NEXT:    [[PP:%.*]] = getelementptr double, double addrspace(2)* [[Q]], i64 [[S]]
+; CHECK-NEXT:    [[L:%.*]] = load double, double addrspace(2)* [[PP]], align 8
+; CHECK-NEXT:    ret double [[L]]
+;
+  %q = addrspacecast double addrspace(1)* %p to double addrspace(2)*
+  %pp = getelementptr double, double addrspace(2)* %q, i32 %i
+  %l = load double, double addrspace(2)* %pp
+  ret double %l
+}