[llvm] Image attribute access for the R600 backend
Matt Arsenault
arsenm2 at gmail.com
Tue Jun 16 11:28:03 PDT 2015
> On Jun 15, 2015, at 11:48 PM, Zoltan Gilian <zoltan.gilian at gmail.com <mailto:zoltan.gilian at gmail.com>> wrote:
>
> Added an intrinsic to load an image attribute stored as an implicit kernel
> argument.
> Added a pass to the R600 backend to replace image attribute getter
> pseudointrinsics to the new image attribute reader intrinsic.
> ---
> include/llvm/IR/IntrinsicsR600.td | 5 +
> lib/Target/R600/AMDGPU.h | 1 +
> lib/Target/R600/AMDGPUTargetMachine.cpp | 1 +
> lib/Target/R600/R600ISelLowering.cpp | 15 ++
> .../R600/R600ImageAttributeIntrinsicsReplacer.cpp | 194 +++++++++++++++++++++
> 5 files changed, 216 insertions(+)
> create mode 100644 lib/Target/R600/R600ImageAttributeIntrinsicsReplacer.cpp
Thanks for working on image features.This patch definitely needs tests. Is this patch supposed to be functional as is, and on what hardware? I think these intrinsics / pass should use the AMDGPU name since they should be generic. There are various formatting problems, you should try running clang-format -style=LLVM on this
>
> diff --git a/include/llvm/IR/IntrinsicsR600.td b/include/llvm/IR/IntrinsicsR600.td
> index 5055667..635cf16 100644
> --- a/include/llvm/IR/IntrinsicsR600.td
> +++ b/include/llvm/IR/IntrinsicsR600.td
> @@ -33,6 +33,11 @@ defm int_r600_read_tgid : R600ReadPreloadRegisterIntrinsic_xyz <
> "__builtin_r600_read_tgid">;
> defm int_r600_read_tidig : R600ReadPreloadRegisterIntrinsic_xyz <
> "__builtin_r600_read_tidig">;
> +
> +def int_r600_read_image_attribute
> + : Intrinsic<[llvm_i32_ty], [llvm_i32_ty, llvm_i32_ty], [IntrNoMem]>,
> + GCCBuiltin<"__builtin_r600_read_image_attribute">;
> +
> } // End TargetPrefix = "r600"
>
> let TargetPrefix = "AMDGPU" in {
> diff --git a/lib/Target/R600/AMDGPU.h b/lib/Target/R600/AMDGPU.h
> index 0a05d25..4b5c5aa 100644
> --- a/lib/Target/R600/AMDGPU.h
> +++ b/lib/Target/R600/AMDGPU.h
> @@ -27,6 +27,7 @@ class TargetMachine;
>
> // R600 Passes
> FunctionPass *createR600VectorRegMerger(TargetMachine &tm);
> +FunctionPass *createR600ImageAttributeIntrinsicsReplacer();
> FunctionPass *createR600TextureIntrinsicsReplacer();
> FunctionPass *createR600ExpandSpecialInstrsPass(TargetMachine &tm);
> FunctionPass *createR600EmitClauseMarkers();
> diff --git a/lib/Target/R600/AMDGPUTargetMachine.cpp b/lib/Target/R600/AMDGPUTargetMachine.cpp
> index 0e37127..0f09a99 100644
> --- a/lib/Target/R600/AMDGPUTargetMachine.cpp
> +++ b/lib/Target/R600/AMDGPUTargetMachine.cpp
> @@ -202,6 +202,7 @@ bool AMDGPUPassConfig::addInstSelector() {
>
> bool R600PassConfig::addPreISel() {
> AMDGPUPassConfig::addPreISel();
> + addPass(createR600ImageAttributeIntrinsicsReplacer());
> addPass(createR600TextureIntrinsicsReplacer());
> return false;
> }
> diff --git a/lib/Target/R600/R600ISelLowering.cpp b/lib/Target/R600/R600ISelLowering.cpp
> index 8357b6d..7bea7117 100644
> --- a/lib/Target/R600/R600ISelLowering.cpp
> +++ b/lib/Target/R600/R600ISelLowering.cpp
> @@ -818,6 +818,21 @@ SDValue R600TargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const
> case Intrinsic::AMDGPU_read_workdim:
> return LowerImplicitParameter(DAG, VT, DL, MFI->ABIArgOffset / 4);
>
> + case Intrinsic::r600_read_image_attribute:
> + {
Brace placement
> + // operand 0: image index
> + // operand 1: attribute index
> +
> + uint64_t DWordOffset = MFI->ABIArgOffset / 4;
> + // Skip grid dim and grid offset.
> + DWordOffset += 4;
> + // There are 5 dword attributes per image.
> + DWordOffset += 5 * Op.getConstantOperandVal(1);
> + // Skip to the requested attribute.
> + DWordOffset += Op.getConstantOperandVal(2);
> + return LowerImplicitParameter(DAG, VT, DL, DWordOffset);
> + }
> +
> case Intrinsic::r600_read_tgid_x:
> return CreateLiveInRegister(DAG, &AMDGPU::R600_TReg32RegClass,
> AMDGPU::T1_X, VT);
> diff --git a/lib/Target/R600/R600ImageAttributeIntrinsicsReplacer.cpp b/lib/Target/R600/R600ImageAttributeIntrinsicsReplacer.cpp
> new file mode 100644
> index 0000000..0439011
> --- /dev/null
> +++ b/lib/Target/R600/R600ImageAttributeIntrinsicsReplacer.cpp
> @@ -0,0 +1,194 @@
> +//===-- R600ImageAttributeIntrinsicsReplacer.cpp --------------------------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +/// \file
> +/// This pass replaces image attribute getter pseudointrinsics with the
> +/// r600_read_image_attribute intrinsic. The pseudointrinsics are used to
> +/// implement OpenCL C get_image_* builtins to avoid using mangled names here.
> +///
> +/// The r600_read_image_attribute intrinsic identifies the image in question
> +/// using an index defined by the following ordering of the image arguments:
> +/// write-only images are ordered before read-only ones; images having the
> +/// same access qualifier follow the order of the image arguments as defined
> +/// by the kernel signature. For each call to an attribute getter the pass
> +/// determines the index of the image operand and passes it to the
> +/// r600_read_image_attribute intrinsic.
> +//===----------------------------------------------------------------------===//
> +
> +#include "AMDGPU.h"
> +#include "llvm/ADT/DenseMap.h"
> +#include "llvm/ADT/SmallBitVector.h"
> +#include "llvm/Analysis/Passes.h"
> +#include "llvm/IR/Function.h"
> +#include "llvm/IR/IRBuilder.h"
> +#include "llvm/IR/InstVisitor.h"
> +#include "llvm/IR/Intrinsics.h"
> +#include "llvm/IR/Metadata.h"
> +
> +using namespace llvm;
> +
> +namespace {
> +
> +// Names of the attribute getter pseudointrinsics.
> +const char * ImageAttributeIntrinsics[] = {
> + "llvm.r600.get.image.width",
> + "llvm.r600.get.image.height",
> + "llvm.r600.get.image.depth",
> + "llvm.r600.get.image.channel.data.type",
> + "llvm.r600.get.image.channel.order"
> + };
> +const unsigned NumImageAttributes = sizeof(ImageAttributeIntrinsics) /
> + sizeof(const char*);
You can use the array_lengthof macro instead. The way this is used, you could also just construct an ArrayRef from this without this constant.
> +
> +class R600ImageAttributeIntrinsicsReplacer :
> + public FunctionPass,
> + public InstVisitor<R600ImageAttributeIntrinsicsReplacer> {
> + static char ID;
> +
> + struct AccessQualInfo {
> + unsigned NumWriteOnlyArgs;
> + SmallBitVector IsWriteOnly;
> + AccessQualInfo(unsigned NumWriteOnlyArgs_, SmallBitVector IsWriteOnly_):
> + NumWriteOnlyArgs(NumWriteOnlyArgs_), IsWriteOnly(IsWriteOnly_) {}
> + };
> +
> + // Per-module state.
> + Type *Int32Type;
> + Function *ReadImageAttributeFun;
> + DenseMap<const Function*, AccessQualInfo> FunctionAccessQualInfo;
> +
> + // Per-function state for visitCallInst.
> + bool modified;
> + DenseMap<const Value*, unsigned> ImageArgIndices;
> +
> +public:
> + R600ImageAttributeIntrinsicsReplacer():
> + FunctionPass(ID) {
> + }
> +
> + bool doInitialization(Module &M) override {
> + Int32Type = Type::getInt32Ty(M.getContext());
> +
> + // Collect image access qualifier metadata.
> + auto KernelsMD = M.getNamedMetadata("opencl.kernels");
> + if (!KernelsMD) return false;
> + for (const MDNode* Op: KernelsMD->operands()) {
> +
> + // Get the access qualifier metadata node.
> + auto KernelFun = mdconst::dyn_extract<Function>(Op->getOperand(0));
> + if (!KernelFun) continue;
Missing newlines
> + auto AccessQualMD = cast<MDNode>(Op->getOperand(2));
> + auto MDName = cast<MDString>(AccessQualMD->getOperand(0));
> + assert(MDName && MDName->getString() == "kernel_arg_access_qual”);
You don’t need to assert MDName. the cast<> already does that
> +
> + // Create a bit vector of write only args and count them.
> + unsigned NumArgs = AccessQualMD->getNumOperands() - 1;
> + SmallBitVector WriteOnlyArgs(NumArgs);
> + unsigned NumWriteOnlyArgs = 0;
> + for (unsigned i = 0; i < NumArgs; ++i) {
> + auto AccessQual = cast<MDString>(AccessQualMD->getOperand(i + 1));
> + bool WriteOnly = AccessQual->getString() == "write_only";
> + if (WriteOnly) NumWriteOnlyArgs++;
> + WriteOnlyArgs[i] = WriteOnly;
> + }
> + auto Info = AccessQualInfo(NumWriteOnlyArgs, WriteOnlyArgs);
> + FunctionAccessQualInfo.insert(std::make_pair(KernelFun, Info));
> + }
> +
> + // Create Function for the image attribute reading intrinsic.
> + ReadImageAttributeFun = Intrinsic::getDeclaration(&M,
> + Intrinsic::r600_read_image_attribute);
> +
> + return true;
> + }
> +
> + bool runOnFunction(Function &F) override {
> +
> + // Try to get access qualifier info.
> + auto InfoIt = FunctionAccessQualInfo.find(&F);
> + if (InfoIt == FunctionAccessQualInfo.end()) return false;
> + auto Info = InfoIt->second;
> +
> + modified = false;
> +
> + // Store indices of image arguments.
> + ImageArgIndices.clear();
> + unsigned NumWriteOnlyArgsSoFar = 0, NumReadOnlyArgsSoFar = 0, i = 0;
> + for (auto arg = F.arg_begin(), E = F.arg_end(); arg != E; ++arg, ++i) {
> +
> + // Skip non-image types.
> + Type *arg_type = arg->getType();
> + if (!arg_type->isPointerTy()) continue;
> + Type *elem_type = arg_type->getPointerElementType();
Variable names should be capitalized and camel case
> + if (!elem_type->isStructTy()) continue;
> + const llvm::StringRef &type_name = elem_type->getStructName();
No need for llvm::. You should also use a StringRef value instead of StringRef&
> + if (!type_name.startswith("opencl.image2d_t") &&
> + !type_name.startswith("opencl.image3d_t")) continue;
> +
> + // Calculate and save image index.
> + // Offset read only image indices by the number of write only ones.
> + unsigned Index = 0;
> + if (Info.IsWriteOnly[i]) {
> + Index = NumWriteOnlyArgsSoFar++;
NumWriteOnlyArgsSoFar could use a better name. How NumWriteOnlyArgs? or NumwriteOnlyArgsSeen?
> + } else {
> + Index = Info.NumWriteOnlyArgs + NumReadOnlyArgsSoFar++;
> + }
> + ImageArgIndices[arg] = Index;
> + }
> +
> + visit(F);
I think instead of visiting all instructions / calls and checking if it is an image intrinsic, it would be better to visit all users of the image arguments. Also this won’t correctly handle image handles passed to other functions. There should probably be a fixme noted somewhere for this.
> +
> + return modified;
> + }
> +
> + void visitCallInst(CallInst &I) {
> + Function* F = I.getCalledFunction();
> + if (!F)
> + return;
> +
> + // Find out if this is a call to one of the pseudointrinsics.
> + StringRef Name = I.getCalledFunction()->getName();
> + unsigned Attribute;
> + for (Attribute = 0; Attribute < NumImageAttributes; Attribute++) {
> + if (Name.startswith(ImageAttributeIntrinsics[Attribute])) {
> + break;
> + }
> + }
> + if (Attribute >= NumImageAttributes) {
> + // Not an image attribute getter pseudointrinsic.
> + return;
> + }
It would be better to factor this out into an isImageIntrinsicCall() function
> +
> + // Get image argument index.
> + auto ImageArg = I.getArgOperand(0);
> + auto ImageArgIt = ImageArgIndices.find(ImageArg);
> + assert(ImageArgIt != ImageArgIndices.end());
> + auto ImageArgIndex = ImageArgIt->second;
> +
> + // Replace instuction with call to r600_read_image_attribute intrinsic.
> + IRBuilder<> Builder(&I);
> + Value* Args[] = { ConstantInt::get(Int32Type, ImageArgIndex),
> + ConstantInt::get(Int32Type, Attribute) };
> + I.replaceAllUsesWith(Builder.CreateCall(ReadImageAttributeFun, Args));
> + I.eraseFromParent();
> + modified = true;
> + }
> +
> + const char *getPassName() const override {
> + return "R600 Image Attribute Intrinsics Replacer";
> + }
> +};
> +
> +char R600ImageAttributeIntrinsicsReplacer::ID = 0;
> +
> +}
> +
> +FunctionPass *llvm::createR600ImageAttributeIntrinsicsReplacer() {
> + return new R600ImageAttributeIntrinsicsReplacer();
> +}
> --
> 2.4.2
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150616/39c67f2f/attachment.html>
More information about the llvm-commits
mailing list