[AMDGPU] [NFC] refactor the AMDGPU attributor

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

413

Lost the comment?

465–466

Reword comment to negate?

[sameerds]

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

413

The "nothing else to do" refers to the rest of the function body, which got moved into checkForQueuePtr(). The short-circuiting logical-or before calling this function is the same thing now.

465–466

Not really. "nothing else to do" simply means "return early" here.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

465–466

amdgpu-queue-ptr isn't an attribute, amdgpu-no-queue-ptr

[sameerds]

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

465–466

Ah, I see. Just like the comment before this one, it is actually referring to "the queue pointer" and not the attribute. I didn't write this code ... git is confused about code that got moved up/down. There are multiple places in this file that still talk about "needs a queue ptr attribute" in the positive sense. Might be good to clean that up separately.

This patch merely moves stuff around and converts the hard-coded mask bits into those generated from enums.

[sameerds]

Fixed stale comments that mention the attribute amdgpu-queue-ptr in the positive sense.

[cfang]

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

400

I don't think we need to checkForQueuePtr when NeedsQueuePtr is
true.

[sameerds]

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

400

Indeed. I am relying on the short-circuiting nature of the logical-or to achieve that.

[sameerds]

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

400

Turns out that this is actually the bitwise operator and it is not short-circuiting. Fixed in D119308. Thanks for catching this!

llvm/lib/Target/AMDGPU/AMDGPUAttributes.def

+//===--- AMDGPUAttributes.def ---------------------------------*- C++ -*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains descriptions of the various function attributes
+// that indicate *absence* of the corresponding implicit kernel
+// arguments.
+//
+//===----------------------------------------------------------------------===//
+
+// NOTE: NO INCLUDE GUARD DESIRED!
+
+AMDGPU_ATTRIBUTE(DISPATCH_PTR, "amdgpu-no-dispatch-ptr")
+AMDGPU_ATTRIBUTE(QUEUE_PTR, "amdgpu-no-queue-ptr")
+AMDGPU_ATTRIBUTE(DISPATCH_ID, "amdgpu-no-dispatch-id")
+AMDGPU_ATTRIBUTE(IMPLICIT_ARG_PTR, "amdgpu-no-implicitarg-ptr")
+AMDGPU_ATTRIBUTE(WORKGROUP_ID_X, "amdgpu-no-workgroup-id-x")
+AMDGPU_ATTRIBUTE(WORKGROUP_ID_Y, "amdgpu-no-workgroup-id-y")
+AMDGPU_ATTRIBUTE(WORKGROUP_ID_Z, "amdgpu-no-workgroup-id-z")
+AMDGPU_ATTRIBUTE(WORKITEM_ID_X, "amdgpu-no-workitem-id-x")
+AMDGPU_ATTRIBUTE(WORKITEM_ID_Y, "amdgpu-no-workitem-id-y")
+AMDGPU_ATTRIBUTE(WORKITEM_ID_Z, "amdgpu-no-workitem-id-z")
+
+#undef AMDGPU_ATTRIBUTE

llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp

 #include "llvm/IR/IntrinsicsR600.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/Attributor.h"
 
 #define DEBUG_TYPE "amdgpu-attributor"
 
 using namespace llvm;
 
+#define AMDGPU_ATTRIBUTE(Name, Str) Name##_POS,
+
+enum ImplicitArgumentPositions {
+  #include "AMDGPUAttributes.def"
+  LAST_ARG_POS
+};
+
+#define AMDGPU_ATTRIBUTE(Name, Str) Name = 1 << Name##_POS,
+
 enum ImplicitArgumentMask {
   NOT_IMPLICIT_INPUT = 0,
-
-  // SGPRs
-  DISPATCH_PTR = 1 << 0,
-  QUEUE_PTR = 1 << 1,
-  DISPATCH_ID = 1 << 2,
-  IMPLICIT_ARG_PTR = 1 << 3,
-  WORKGROUP_ID_X = 1 << 4,
-  WORKGROUP_ID_Y = 1 << 5,
-  WORKGROUP_ID_Z = 1 << 6,
-
-  // VGPRS:
-  WORKITEM_ID_X = 1 << 7,
-  WORKITEM_ID_Y = 1 << 8,
-  WORKITEM_ID_Z = 1 << 9,
-  ALL_ARGUMENT_MASK = (1 << 10) - 1
+  #include "AMDGPUAttributes.def"
+  ALL_ARGUMENT_MASK = (1 << LAST_ARG_POS) - 1
 };
 
+#define AMDGPU_ATTRIBUTE(Name, Str) {Name, Str},
 static constexpr std::pair<ImplicitArgumentMask,
                            StringLiteral> ImplicitAttrs[] = {
-  {DISPATCH_PTR, "amdgpu-no-dispatch-ptr"},
-  {QUEUE_PTR, "amdgpu-no-queue-ptr"},
-  {DISPATCH_ID, "amdgpu-no-dispatch-id"},
-  {IMPLICIT_ARG_PTR, "amdgpu-no-implicitarg-ptr"},
-  {WORKGROUP_ID_X, "amdgpu-no-workgroup-id-x"},
-  {WORKGROUP_ID_Y, "amdgpu-no-workgroup-id-y"},
-  {WORKGROUP_ID_Z, "amdgpu-no-workgroup-id-z"},
-  {WORKITEM_ID_X, "amdgpu-no-workitem-id-x"},
-  {WORKITEM_ID_Y, "amdgpu-no-workitem-id-y"},
-  {WORKITEM_ID_Z, "amdgpu-no-workitem-id-z"}
+ #include "AMDGPUAttributes.def"
 };
 
 // We do not need to note the x workitem or workgroup id because they are always
 // initialized.
 //
 // TODO: We should not add the attributes if the known compile time workgroup
 // size is 1 for y/z.
 static ImplicitArgumentMask
case Intrinsic::amdgcn_dispatch_ptr:
     return DISPATCH_PTR;
   case Intrinsic::amdgcn_dispatch_id:
     return DISPATCH_ID;
   case Intrinsic::amdgcn_implicitarg_ptr:
     return IMPLICIT_ARG_PTR;
   case Intrinsic::amdgcn_queue_ptr:
   case Intrinsic::amdgcn_is_shared:
   case Intrinsic::amdgcn_is_private:
-    // TODO: Does not require queue ptr on gfx9+
+    // TODO: Does not require the queue pointer on gfx9+
   case Intrinsic::trap:
   case Intrinsic::debugtrap:
     IsQueuePtr = true;
     return QUEUE_PTR;
   default:
     return NOT_IMPLICIT_INPUT;
   }
 }
public:
 
   std::pair<unsigned, unsigned>
   getMaximumFlatWorkGroupRange(const Function &F) {
     const GCNSubtarget &ST = TM.getSubtarget<GCNSubtarget>(F);
     return {ST.getMinFlatWorkGroupSize(), ST.getMaxFlatWorkGroupSize()};
   }
 
 private:
-  /// Check if the ConstantExpr \p CE requires queue ptr attribute.
+  /// Check if the ConstantExpr \p CE requires the queue pointer.
   static bool visitConstExpr(const ConstantExpr *CE) {
     if (CE->getOpcode() == Instruction::AddrSpaceCast) {
       unsigned SrcAS = CE->getOperand(0)->getType()->getPointerAddressSpace();
       return castRequiresQueuePtr(SrcAS);
     }
     return false;
   }
 
for (const Use &U : C->operands()) {
         continue;
 
       Result |= getConstantAccess(OpC);
     }
     return Result;
   }
 
 public:
-  /// Returns true if \p Fn needs a queue ptr attribute because of \p C.
+  /// Returns true if \p Fn needs the queue pointer because of \p C.
   bool needsQueuePtr(const Constant *C, Function &Fn) {
     bool IsNonEntryFunc = !AMDGPU::isEntryFunctionCC(Fn.getCallingConv());
     bool HasAperture = hasApertureRegs(Fn);
 
     // No need to explore the constants.
     if (!IsNonEntryFunc && HasAperture)
       return false;
 
     uint8_t Access = getConstantAccess(C);
 
     // We need to trap on DS globals in non-entry functions.
     if (IsNonEntryFunc && (Access & DS_GLOBAL))
       return true;
 
     return !HasAperture && (Access & ADDR_SPACE_CAST);
   }
 
 private:
-  /// Used to determine if the Constant needs a queue ptr attribute.
+  /// Used to determine if the Constant needs the queue pointer.
   DenseMap<const Constant *, uint8_t> ConstantStatus;
 };
 
 struct AAAMDAttributes : public StateWrapper<
   BitIntegerState<uint16_t, ALL_ARGUMENT_MASK, 0>, AbstractAttribute> {
   using Base = StateWrapper<BitIntegerState<uint16_t, ALL_ARGUMENT_MASK, 0>,
                             AbstractAttribute>;
 
ChangeStatus updateImpl(Attributor &A) override {
 
     // Check for Intrinsics and propagate attributes.
     const AACallEdges &AAEdges = A.getAAFor<AACallEdges>(
         *this, this->getIRPosition(), DepClassTy::REQUIRED);
     if (AAEdges.hasNonAsmUnknownCallee())
       return indicatePessimisticFixpoint();
 
     bool IsNonEntryFunc = !AMDGPU::isEntryFunctionCC(F->getCallingConv());
-    auto &InfoCache = static_cast<AMDGPUInformationCache &>(A.getInfoCache());
 
     bool NeedsQueuePtr = false;
 
     for (Function *Callee : AAEdges.getOptimisticEdges()) {
       Intrinsic::ID IID = Callee->getIntrinsicID();
       if (IID == Intrinsic::not_intrinsic) {
         const AAAMDAttributes &AAAMD = A.getAAFor<AAAMDAttributes>(
           *this, IRPosition::function(*Callee), DepClassTy::REQUIRED);
         *this &= AAAMD;
         continue;
       }
 
       bool NonKernelOnly = false;
       ImplicitArgumentMask AttrMask =
           intrinsicToAttrMask(IID, NonKernelOnly, NeedsQueuePtr);
       if (AttrMask != NOT_IMPLICIT_INPUT) {
         if ((IsNonEntryFunc || !NonKernelOnly))
           removeAssumedBits(AttrMask);
       }
     }
 
-    // If we found that we need amdgpu-queue-ptr, nothing else to do.
+    NeedsQueuePtr |= checkForQueuePtr(A);
     if (NeedsQueuePtr) {
       removeAssumedBits(QUEUE_PTR);
-      return getAssumed() != OrigAssumed ? ChangeStatus::CHANGED :
-                                           ChangeStatus::UNCHANGED;
     }
 
+    return getAssumed() != OrigAssumed ? ChangeStatus::CHANGED
+                                       : ChangeStatus::UNCHANGED;
+  }
+
+  ChangeStatus manifest(Attributor &A) override {
+    SmallVector<Attribute, 8> AttrList;
+    LLVMContext &Ctx = getAssociatedFunction()->getContext();
+
+    for (auto Attr : ImplicitAttrs) {
+      if (isKnown(Attr.first))
+        AttrList.push_back(Attribute::get(Ctx, Attr.second));
+    }
+
+    return IRAttributeManifest::manifestAttrs(A, getIRPosition(), AttrList,
+                                              /* ForceReplace */ true);
+  }
+
+  const std::string getAsStr() const override {
+    std::string Str;
+    raw_string_ostream OS(Str);
+    OS << "AMDInfo[";
+    for (auto Attr : ImplicitAttrs)
+      OS << ' ' << Attr.second;
+    OS << " ]";
+    return OS.str();
+  }
+
+  /// See AbstractAttribute::trackStatistics()
+  void trackStatistics() const override {}
+
+private:
+  bool checkForQueuePtr(Attributor &A) {
+    Function *F = getAssociatedFunction();
+    bool IsNonEntryFunc = !AMDGPU::isEntryFunctionCC(F->getCallingConv());
+
+    auto &InfoCache = static_cast<AMDGPUInformationCache &>(A.getInfoCache());
+
+    bool NeedsQueuePtr = false;
+
     auto CheckAddrSpaceCasts = [&](Instruction &I) {
       unsigned SrcAS = static_cast<AddrSpaceCastInst &>(I).getSrcAddressSpace();
       if (castRequiresQueuePtr(SrcAS)) {
         NeedsQueuePtr = true;
         return false;
       }
       return true;
     };
 
     bool HasApertureRegs = InfoCache.hasApertureRegs(*F);
 
     // `checkForAllInstructions` is much more cheaper than going through all
     // instructions, try it first.
 
-    // amdgpu-queue-ptr is not needed if aperture regs is present.
+    // The queue pointer is not needed if aperture regs is present.
     if (!HasApertureRegs) {
       bool UsedAssumedInformation = false;
       A.checkForAllInstructions(CheckAddrSpaceCasts, *this,
                                 {Instruction::AddrSpaceCast},
                                 UsedAssumedInformation);
     }
 
-    // If we found  that we need amdgpu-queue-ptr, nothing else to do.
-    if (NeedsQueuePtr) {
-      removeAssumedBits(QUEUE_PTR);
-      return getAssumed() != OrigAssumed ? ChangeStatus::CHANGED :
-                                           ChangeStatus::UNCHANGED;
-    }
+    // If we found  that we need the queue pointer, nothing else to do.
+    if (NeedsQueuePtr)
+      return true;
 
-    if (!IsNonEntryFunc && HasApertureRegs) {
-      return getAssumed() != OrigAssumed ? ChangeStatus::CHANGED :
-                                           ChangeStatus::UNCHANGED;
-    }
+    if (!IsNonEntryFunc && HasApertureRegs)
+      return false;
 
     for (BasicBlock &BB : *F) {
       for (Instruction &I : BB) {
         for (const Use &U : I.operands()) {
           if (const auto *C = dyn_cast<Constant>(U)) {
-            if (InfoCache.needsQueuePtr(C, *F)) {
-              removeAssumedBits(QUEUE_PTR);
-              return getAssumed() != OrigAssumed ? ChangeStatus::CHANGED :
-                                                   ChangeStatus::UNCHANGED;
-            }
+            if (InfoCache.needsQueuePtr(C, *F))
+              return true;
           }
         }
       }
     }
 
-    return getAssumed() != OrigAssumed ? ChangeStatus::CHANGED :
-                                         ChangeStatus::UNCHANGED;
-  }
-
-  ChangeStatus manifest(Attributor &A) override {
-    SmallVector<Attribute, 8> AttrList;
-    LLVMContext &Ctx = getAssociatedFunction()->getContext();
-
-    for (auto Attr : ImplicitAttrs) {
-      if (isKnown(Attr.first))
-        AttrList.push_back(Attribute::get(Ctx, Attr.second));
-    }
-
-    return IRAttributeManifest::manifestAttrs(A, getIRPosition(), AttrList,
-                                              /* ForceReplace */ true);
-  }
-
-  const std::string getAsStr() const override {
-    std::string Str;
-    raw_string_ostream OS(Str);
-    OS << "AMDInfo[";
-    for (auto Attr : ImplicitAttrs)
-      OS << ' ' << Attr.second;
-    OS << " ]";
-    return OS.str();
+    return false;
   }
-
-  /// See AbstractAttribute::trackStatistics()
-  void trackStatistics() const override {}
 };
 
 AAAMDAttributes &AAAMDAttributes::createForPosition(const IRPosition &IRP,
                                                     Attributor &A) {
   if (IRP.getPositionKind() == IRPosition::IRP_FUNCTION)
     return *new (A.Allocator) AAAMDAttributesFunction(IRP, A);
   llvm_unreachable("AAAMDAttributes is only valid for function position");
 }