[NVPTX] Add an "image" attribute and simplify the NVPTX rdoimage, wroimage, and rdrwimage attributes.

[brianagrace]

Summary

This is the first in a series of patches to simplify the way NVPTX attaches metadata to functions, function arguments, and global variables. Currently all NVPTX metadata is found via the !nvvm.annotations module-level metadata. This means that reading metadata for one function, argument, or global variable requires iterating over *all* of the module's NVPTX metadata. (We have a janky, global cache that attempts to fix this, but doesn't really succeed.)

We aim to incrementally change the NVPTX metadata format so that our annotations are attached to the objects they describe. This will simplify the format and obviate the need for the cache.

NVPTX has the notion of "image" arguments to kernel functions. These correspond to the .surfref and .texref argument annotations [1], and whether or not an argument is an image is used in NVPTXImageOptimizer. An image argument may be either read-only, read-write, or write-only. This patch:

• Adds an LLVM "image" attribute,
• Lets us mark non-pointers as read-only / write-only (images can be i64s), and
• Changes the NVPTX "isImage" functions to recognize the new format, e.g. "define void @foo(i64 readonly image %img)".

[1] http://docs.nvidia.com/cuda/parallel-thread-execution/#texture-sampler-and-surface-types

[brianagrace]

• • Added new Image attribute
  • Had to change readonly attribute to apply to types other than pointers - not sure if that's a good idea
• Still have to update llvm documentation

[jlebar]

We should elaborate more in the patch description with respect to (a) what we're changing (may need to describe the current and new state) and (b) why we're changing it. We should also pay special attention to the first line of the patch description (I think in phab it's the "title") because that's all some people will see.

Maybe something like:

[NVPTX] Add an "image" attribute and simplify the NVPTX rdoimage, wroimage, and rdrwimage attributes.

This is the first in a series of patches to simplify the way NVPTX attaches metadata to functions, function arguments, and global variables. Currently all NVPTX metadata is found via the !nvvm.annotations module-level metadata. This means that reading metadata for one function, argument, or global variable requires iterating over *all* of the module's NVPTX metadata. (We have a janky, global cache that attempts to fix this, but doesn't really succeed.)

We aim to incrementally change the NVPTX metadata format so that our annotations are attached to the objects they describe. This will simplify the format and obviate the need for the cache.

NVPTX has the notion of "image" arguments to kernel functions. These correspond to the .surfref and .texref argument annotations [1], and whether or not an argument is an image is used in NVPTXImageOptimizer. An image argument may be either read-only, read-write, or write-only. This patch:

• adds an LLVM "image" attribute,
• lets us mark non-pointers as read-only / write-only (images can be i64s), and
• changes the NVPTX "isImage" functions to recognize the new format, e.g. "define void @foo(i64 readonly image %img)".

[1] http://docs.nvidia.com/cuda/parallel-thread-execution/#texture-sampler-and-surface-types

include/llvm/IR/Attributes.td

48

Update comment?

[brianagrace]

• List Item

[jlebar]

Be sure to add llvm-commits when creating phab reviews, otherwise the mailing list won't see it and people will be sad. (I have no idea why we cannot do this automatically.)

[jlebar]

include/llvm/IR/Attributes.td

48

@tra @arsenm, Do you think we should call this "texref", "textureref" or something? AFAICT what nvptx calls an "image" really means "a texture or surface ref", where the difference between texture and surface refs is that surface refs are writable.

lib/Target/NVPTX/NVPTXUtilities.cpp

-17

Probably didn't mean to make this change?

177

Need to check that it's also an image?

test/CodeGen/NVPTX/surf-read.ll

20

(Just as an example of how messed up this old metadata format is: Apparently "i32 0" here means that this *is* an image? Like, it looks like it's saying "false"!)

[jlebar]

An earlier commit description had said that you needed to make readonly /writeonly work on non-pointer types -- is that change still necessary? I expect it will be somewhat controversial...

[tra]

I'd say texref. CUDA appears to shorten it this way in the driver API where we have number of functions with cuTexRef prefix.

[arsenm]

What advantage over reserving an address space for images does this have? Now that we can have non-integral pointer types, why not just designate an address space for read/write images?

[jlebar]

In D30669#695229, @arsenm wrote:

What advantage over reserving an address space for images does this have? Now that we can have non-integral pointer types, why not just designate an address space for read/write images?

Hm, I guess that makes a lot of sense... I don't think it's legal to GEP, address space cast, or even do regular loads on these pointers, but whatever?

I'm a bit concerned that making this change will require changing all of the NVPTX texture and surface intrinsics, which is kind of a tall order and may have add-on consequences. Is there a reasonable intermediate state?

Maybe we should just drop support for image args to kernels until we have a frontend which uses this. It seems like otherwise, if we make a big change, we're somewhat likely to have to make another big change when we actually add front-end support.

WDYT?

[brianagrace]

Adressing comments

test/CodeGen/NVPTX/surf-read.ll

20

Its the argument number - the 0th arg of the function is an image

[arsenm]

In D30669#695232, @jlebar wrote:

In D30669#695229, @arsenm wrote:

What advantage over reserving an address space for images does this have? Now that we can have non-integral pointer types, why not just designate an address space for read/write images?

Hm, I guess that makes a lot of sense... I don't think it's legal to GEP, address space cast, or even do regular loads on these pointers, but whatever?

I think these are allowed, just not ptrtoint

include/llvm/Bitcode/LLVMBitCodes.h

enum AttributeKindCodes {
   ATTR_KIND_SAFESTACK = 44,
   ATTR_KIND_ARGMEMONLY = 45,
   ATTR_KIND_SWIFT_SELF = 46,
   ATTR_KIND_SWIFT_ERROR = 47,
   ATTR_KIND_NO_RECURSE = 48,
   ATTR_KIND_INACCESSIBLEMEM_ONLY = 49,
   ATTR_KIND_INACCESSIBLEMEM_OR_ARGMEMONLY = 50,
   ATTR_KIND_ALLOC_SIZE = 51,
-  ATTR_KIND_WRITEONLY = 52
+  ATTR_KIND_WRITEONLY = 52,
+  ATTR_KIND_IMAGE = 53
 };
 
 enum ComdatSelectionKindCodes {
   COMDAT_SELECTION_KIND_ANY = 1,
   COMDAT_SELECTION_KIND_EXACT_MATCH = 2,
   COMDAT_SELECTION_KIND_LARGEST = 3,
   COMDAT_SELECTION_KIND_NO_DUPLICATES = 4,
   COMDAT_SELECTION_KIND_SAME_SIZE = 5,
 };
 
 } // End bitc namespace
 } // End llvm namespace
 
 #endif

include/llvm/IR/Attributes.td

 def Convergent : EnumAttr<"convergent">;
 
 /// Pointer is known to be dereferenceable.
 def Dereferenceable : EnumAttr<"dereferenceable">;
 
 /// Pointer is either null or dereferenceable.
 def DereferenceableOrNull : EnumAttr<"dereferenceable_or_null">;
 
+/// Argument is an image.
+def Image : EnumAttr<"image">;
+
 /// Function may only access memory that is inaccessible from IR.
 def InaccessibleMemOnly : EnumAttr<"inaccessiblememonly">;
 
 /// Function may only access memory that is either inaccessible from the IR,
 /// or pointed to by its pointer arguments.
 def InaccessibleMemOrArgMemOnly : EnumAttr<"inaccessiblemem_or_argmemonly">;
 
 /// Pass structure in an alloca.

lib/AsmParser/LLLexer.cpp

#define KEYWORD(STR)                                                           \
   KEYWORD(argmemonly);
   KEYWORD(builtin);
   KEYWORD(byval);
   KEYWORD(inalloca);
   KEYWORD(cold);
   KEYWORD(convergent);
   KEYWORD(dereferenceable);
   KEYWORD(dereferenceable_or_null);
+  KEYWORD(image);
   KEYWORD(inaccessiblememonly);
   KEYWORD(inaccessiblemem_or_argmemonly);
   KEYWORD(inlinehint);
   KEYWORD(inreg);
   KEYWORD(jumptable);
   KEYWORD(minsize);
   KEYWORD(naked);
   KEYWORD(nest);

lib/AsmParser/LLParser.cpp

case lltok::kw_allocsize: {
       B.addAllocSizeAttr(ElemSizeArg, NumElemsArg);
       continue;
     }
     case lltok::kw_alwaysinline: B.addAttribute(Attribute::AlwaysInline); break;
     case lltok::kw_argmemonly: B.addAttribute(Attribute::ArgMemOnly); break;
     case lltok::kw_builtin: B.addAttribute(Attribute::Builtin); break;
     case lltok::kw_cold: B.addAttribute(Attribute::Cold); break;
     case lltok::kw_convergent: B.addAttribute(Attribute::Convergent); break;
+    case lltok::kw_image: B.addAttribute(Attribute::Image); break;
     case lltok::kw_inaccessiblememonly:
       B.addAttribute(Attribute::InaccessibleMemOnly); break;
     case lltok::kw_inaccessiblemem_or_argmemonly:
       B.addAttribute(Attribute::InaccessibleMemOrArgMemOnly); break;
     case lltok::kw_inlinehint: B.addAttribute(Attribute::InlineHint); break;
     case lltok::kw_jumptable: B.addAttribute(Attribute::JumpTable); break;
     case lltok::kw_minsize: B.addAttribute(Attribute::MinSize); break;
     case lltok::kw_naked: B.addAttribute(Attribute::Naked); break;
while (true) {
     }
     case lltok::kw_dereferenceable_or_null: {
       uint64_t Bytes;
       if (ParseOptionalDerefAttrBytes(lltok::kw_dereferenceable_or_null, Bytes))
         return true;
       B.addDereferenceableOrNullAttr(Bytes);
       continue;
     }
+    case lltok::kw_image:           B.addAttribute(Attribute::Image); break;
     case lltok::kw_inalloca:        B.addAttribute(Attribute::InAlloca); break;
     case lltok::kw_inreg:           B.addAttribute(Attribute::InReg); break;
     case lltok::kw_nest:            B.addAttribute(Attribute::Nest); break;
     case lltok::kw_noalias:         B.addAttribute(Attribute::NoAlias); break;
     case lltok::kw_nocapture:       B.addAttribute(Attribute::NoCapture); break;
     case lltok::kw_nonnull:         B.addAttribute(Attribute::NonNull); break;
     case lltok::kw_readnone:        B.addAttribute(Attribute::ReadNone); break;
     case lltok::kw_readonly:        B.addAttribute(Attribute::ReadOnly); break;

lib/AsmParser/LLToken.h

enum Kind {
   kw_sanitize_address,
   kw_builtin,
   kw_byval,
   kw_inalloca,
   kw_cold,
   kw_convergent,
   kw_dereferenceable,
   kw_dereferenceable_or_null,
+  kw_image,
   kw_inaccessiblememonly,
   kw_inaccessiblemem_or_argmemonly,
   kw_inlinehint,
   kw_inreg,
   kw_jumptable,
   kw_minsize,
   kw_naked,
   kw_nest,

lib/Bitcode/Reader/BitcodeReader.cpp

static uint64_t getRawAttributeMask(Attribute::AttrKind Val) {
   case Attribute::Convergent:      return 1ULL << 46;
   case Attribute::SafeStack:       return 1ULL << 47;
   case Attribute::NoRecurse:       return 1ULL << 48;
   case Attribute::InaccessibleMemOnly:         return 1ULL << 49;
   case Attribute::InaccessibleMemOrArgMemOnly: return 1ULL << 50;
   case Attribute::SwiftSelf:       return 1ULL << 51;
   case Attribute::SwiftError:      return 1ULL << 52;
   case Attribute::WriteOnly:       return 1ULL << 53;
+  case Attribute::Image:           return 1ULL << 54;
   case Attribute::Dereferenceable:
     llvm_unreachable("dereferenceable attribute not supported in raw format");
     break;
   case Attribute::DereferenceableOrNull:
     llvm_unreachable("dereferenceable_or_null attribute not supported in raw "
                      "format");
     break;
   case Attribute::ArgMemOnly:
static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
   case bitc::ATTR_KIND_BY_VAL:
     return Attribute::ByVal;
   case bitc::ATTR_KIND_IN_ALLOCA:
     return Attribute::InAlloca;
   case bitc::ATTR_KIND_COLD:
     return Attribute::Cold;
   case bitc::ATTR_KIND_CONVERGENT:
     return Attribute::Convergent;
+  case bitc::ATTR_KIND_IMAGE:
+    return Attribute::Image;
   case bitc::ATTR_KIND_INACCESSIBLEMEM_ONLY:
     return Attribute::InaccessibleMemOnly;
   case bitc::ATTR_KIND_INACCESSIBLEMEM_OR_ARGMEMONLY:
     return Attribute::InaccessibleMemOrArgMemOnly;
   case bitc::ATTR_KIND_INLINE_HINT:
     return Attribute::InlineHint;
   case bitc::ATTR_KIND_IN_REG:
     return Attribute::InReg;

lib/Bitcode/Writer/BitcodeWriter.cpp

static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   case Attribute::Convergent:
     return bitc::ATTR_KIND_CONVERGENT;
   case Attribute::InAlloca:
     return bitc::ATTR_KIND_IN_ALLOCA;
   case Attribute::Cold:
     return bitc::ATTR_KIND_COLD;
   case Attribute::InaccessibleMemOnly:
     return bitc::ATTR_KIND_INACCESSIBLEMEM_ONLY;
+  case Attribute::Image:
+    return bitc::ATTR_KIND_IMAGE;
   case Attribute::InaccessibleMemOrArgMemOnly:
     return bitc::ATTR_KIND_INACCESSIBLEMEM_OR_ARGMEMONLY;
   case Attribute::InlineHint:
     return bitc::ATTR_KIND_INLINE_HINT;
   case Attribute::InReg:
     return bitc::ATTR_KIND_IN_REG;
   case Attribute::JumpTable:
     return bitc::ATTR_KIND_JUMP_TABLE;

lib/IR/Attributes.cpp

std::string Attribute::getAsString(bool InAttrGrp) const {
   if (hasAttribute(Attribute::ByVal))
     return "byval";
   if (hasAttribute(Attribute::Convergent))
     return "convergent";
   if (hasAttribute(Attribute::SwiftError))
     return "swifterror";
   if (hasAttribute(Attribute::SwiftSelf))
     return "swiftself";
+  if (hasAttribute(Attribute::Image))
+    return "image";
   if (hasAttribute(Attribute::InaccessibleMemOnly))
     return "inaccessiblememonly";
   if (hasAttribute(Attribute::InaccessibleMemOrArgMemOnly))
     return "inaccessiblemem_or_argmemonly";
   if (hasAttribute(Attribute::InAlloca))
     return "inalloca";
   if (hasAttribute(Attribute::InlineHint))
     return "inlinehint";
if (!Ty->isPointerTy())
     Incompatible.addAttribute(Attribute::ByVal)
       .addAttribute(Attribute::Nest)
       .addAttribute(Attribute::NoAlias)
       .addAttribute(Attribute::NoCapture)
       .addAttribute(Attribute::NonNull)
       .addDereferenceableAttr(1) // the int here is ignored
       .addDereferenceableOrNullAttr(1) // the int here is ignored
       .addAttribute(Attribute::ReadNone)
-      .addAttribute(Attribute::ReadOnly)
       .addAttribute(Attribute::StructRet)
       .addAttribute(Attribute::InAlloca);
 
   return Incompatible;
 }
 
 template<typename AttrClass>
 static bool isEqual(const Function &Caller, const Function &Callee) {

lib/Target/NVPTX/NVPTXUtilities.cpp

 //===- NVPTXUtilities.cpp - Utility Functions -----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
if (findAllNVVMAnnotation(func, AnnotationName, annot)) {
       if (is_contained(annot, arg->getArgNo()))
         return true;
     }
   }
   return false;
 }
 
 bool isImageReadOnly(const Value &val) {
-  if (const Argument *arg = dyn_cast<Argument>(&val)) {
-    const Function *func = arg->getParent();
-    std::vector<unsigned> annot;
-    if (findAllNVVMAnnotation(func, "rdoimage", annot)) {
-      if (is_contained(annot, arg->getArgNo()))
-        return true;
-    }
-  }
+  if (const Argument *Arg = dyn_cast<Argument>(&val))
+    return isImage(val) && Arg->hasAttribute(Attribute::ReadOnly);
   return false;
 }
 
 bool isImageWriteOnly(const Value &val) {
-  if (const Argument *arg = dyn_cast<Argument>(&val)) {
-    const Function *func = arg->getParent();
-    std::vector<unsigned> annot;
-    if (findAllNVVMAnnotation(func, "wroimage", annot)) {
-      if (is_contained(annot, arg->getArgNo()))
-        return true;
-    }
-  }
+  if (const Argument *Arg = dyn_cast<Argument>(&val))
+    return isImage(val) && Arg->hasAttribute(Attribute::WriteOnly);
   return false;
 }
 
 bool isImageReadWrite(const Value &val) {
-  if (const Argument *arg = dyn_cast<Argument>(&val)) {
-    const Function *func = arg->getParent();
-    std::vector<unsigned> annot;
-    if (findAllNVVMAnnotation(func, "rdwrimage", annot)) {
-      if (is_contained(annot, arg->getArgNo()))
-        return true;
-    }
-  }
-  return false;
+  return isImage(val) && !isImageReadOnly(val) && !isImageWriteOnly(val);
 }
 
 bool isImage(const Value &val) {
-  return isImageReadOnly(val) || isImageWriteOnly(val) || isImageReadWrite(val);
+  if (const Argument *Arg = dyn_cast<Argument>(&val))
+    return Arg->hasAttribute(Attribute::Image);
+  return false;
 }
 
 bool isManaged(const Value &val) {
   if(const GlobalValue *gv = dyn_cast<GlobalValue>(&val)) {
     unsigned annot;
     if (findOneNVVMAnnotation(gv, "managed", annot)) {
       assert((annot == 1) && "Unexpected annotation on a managed symbol");
       return true;

test/CodeGen/NVPTX/nvcl-param-align.ll

 ; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s
 
 target triple = "nvptx-unknown-nvcl"
 
 ; CHECK-LABEL: .entry foo(
-define void @foo(i64 %img, i64 %sampler, <5 x float>* %v) {
+define void @foo(i64 readonly image %img, i64 %sampler, <5 x float>* %v) {
 ; The parameter alignment should be the next power of 2 of 5xsizeof(float),
 ; which is 32.
 ; CHECK: .param .u32 .ptr .align 32 foo_param_2
   ret void
 }
 
-!nvvm.annotations = !{!1, !2, !3}
+!nvvm.annotations = !{!1, !2}
 !1 = !{void (i64, i64, <5 x float>*)* @foo, !"kernel", i32 1}
-!2 = !{void (i64, i64, <5 x float>*)* @foo, !"rdoimage", i32 0}
-!3 = !{void (i64, i64, <5 x float>*)* @foo, !"sampler", i32 1}
+!2 = !{void (i64, i64, <5 x float>*)* @foo, !"sampler", i32 1}

test/CodeGen/NVPTX/surf-read.ll

 ; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s
 
 target triple = "nvptx-unknown-nvcl"
 
 declare i32 @llvm.nvvm.suld.1d.i32.trap(i64, i32)
 
 ; CHECK: .entry foo
-define void @foo(i64 %img, float* %red, i32 %idx) {
+define void @foo(i64 image %img, float* %red, i32 %idx) {
 ; CHECK: suld.b.1d.b32.trap {%r[[RED:[0-9]+]]}, [foo_param_0, {%r{{[0-9]+}}}]
   %val = tail call i32 @llvm.nvvm.suld.1d.i32.trap(i64 %img, i32 %idx)
 ; CHECK: cvt.rn.f32.s32 %f[[REDF:[0-9]+]], %r[[RED]]
   %ret = sitofp i32 %val to float
 ; CHECK: st.f32 [%r{{[0-9]+}}], %f[[REDF]]
   store float %ret, float* %red
   ret void
 }
 
-!nvvm.annotations = !{!1, !2}
+!nvvm.annotations = !{!1}
 !1 = !{void (i64, float*, i32)* @foo, !"kernel", i32 1}
-!2 = !{void (i64, float*, i32)* @foo, !"rdwrimage", i32 0}

test/CodeGen/NVPTX/surf-write.ll

 ; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s
 
 target triple = "nvptx-unknown-nvcl"
 
 declare void @llvm.nvvm.sust.b.1d.i32.trap(i64, i32, i32)
 
 ; CHECK: .entry foo
-define void @foo(i64 %img, i32 %val, i32 %idx) {
+define void @foo(i64 writeonly image %img, i32 %val, i32 %idx) {
 ; CHECK: sust.b.1d.b32.trap [foo_param_0, {%r{{[0-9]+}}}], {%r{{[0-9]+}}}
   tail call void @llvm.nvvm.sust.b.1d.i32.trap(i64 %img, i32 %idx, i32 %val)
   ret void
 }
 
-!nvvm.annotations = !{!1, !2}
+!nvvm.annotations = !{!1}
 !1 = !{void (i64, i32, i32)* @foo, !"kernel", i32 1}
-!2 = !{void (i64, i32, i32)* @foo, !"wroimage", i32 0}

test/CodeGen/NVPTX/tex-read.ll

 ; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s
 
 target triple = "nvptx-unknown-nvcl"
 
 declare { float, float, float, float } @llvm.nvvm.tex.1d.v4f32.s32(i64, i64, i32)
 
 ; CHECK: .entry foo
-define void @foo(i64 %img, i64 %sampler, float* %red, i32 %idx) {
+define void @foo(i64 readonly image %img, i64 %sampler, float* %red, i32 %idx) {
 ; CHECK: tex.1d.v4.f32.s32 {%f[[RED:[0-9]+]], %f[[GREEN:[0-9]+]], %f[[BLUE:[0-9]+]], %f[[ALPHA:[0-9]+]]}, [foo_param_0, foo_param_1, {%r{{[0-9]+}}}]
   %val = tail call { float, float, float, float } @llvm.nvvm.tex.1d.v4f32.s32(i64 %img, i64 %sampler, i32 %idx)
   %ret = extractvalue { float, float, float, float } %val, 0
 ; CHECK: st.f32 [%r{{[0-9]+}}], %f[[RED]]
   store float %ret, float* %red
   ret void
 }
 
-!nvvm.annotations = !{!1, !2, !3}
+!nvvm.annotations = !{!1, !2}
 !1 = !{void (i64, i64, float*, i32)* @foo, !"kernel", i32 1}
-!2 = !{void (i64, i64, float*, i32)* @foo, !"rdoimage", i32 0}
-!3 = !{void (i64, i64, float*, i32)* @foo, !"sampler", i32 1}
+!2 = !{void (i64, i64, float*, i32)* @foo, !"sampler", i32 1}