[PATCH] D102066: [AMDGPU] Fix codegen of image intrinsics for g16 and a16
Jay Foad via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 7 05:00:56 PDT 2021
foad added a comment.
Does globalisel get this right? Maybe add a -global-isel RUN line to the new test?
================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6108-6109
+ dbgs()
+ << "Failed to lower image intrinsic: 16 bit addresses "
+ "need 16 bit addresses for 16 bit derivatives (got 32 bit)");
+ return Op;
----------------
"16 bit addresses need 16 bit addresses ..." reads wrong. But anyway maybe combine these two cases into a single "if (IsA16 != IsG16)" with an appropriately general error message?
================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6127
+ // gradient or both)
+ if (BaseOpcode->Gradients && IsG16 && !IsTiedG16) {
+ // Activate g16
----------------
Might be simpler to remove IsTiedG16 and just test `!ST->hasG16()` again here.
================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6138
+ // const int PackEndIdx = IsA16 ? VAddrEnd : (ArgOffset + Intr->CoordStart);
+ packImageA16AddressToDwords(
+ DAG, Op, GradPackVectorVT, VAddrs, ArgOffset + Intr->GradientStart,
----------------
Seems like there are some future cleanup opportunities here: packImageA16AddressToDwords shouldn't really have "A16" in its name since it's used for G16 too, and maybe A16/G16 should be passed in as an argument so the "if (A16)" test can be inside the helper function.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102066/new/
https://reviews.llvm.org/D102066
More information about the llvm-commits
mailing list