[PATCH] D73446: AMDGPU/GlobalISel: Legalize a16 images

Tue Mar 17 03:46:55 PDT 2020

nhaehnle accepted this revision.
nhaehnle added a comment.
This revision is now accepted and ready to land.

>> The thing under discussion here from my perspective is that it's awkward to overload the semantics of image intrinsics in the way that this and related changes are doing, and the question was why we can't just directly go to the final image instructions. One aspect of this is that you'd have a non-generic machine instruction refering to register that don't have a register class, for a couple of passes at least. That doesn't seem too crazy to me.
> 
> I'm leaning towards inventing what is essentially a custom G_INTRINSIC type to track the legalization of the awkward cases. The important information will still be tracked by preserving the intrinsic ID operand, but the operands will be changed as here. I think this only requires a small number of wrapper operations (I think 1, but maybe 4 at most). The current intermediate DAG nodes seem to get away with just _d16 variants for dealing with the annoying unpacked register layout case.

I like this idea. I can see how this could be considered a change that is separate from this change, so this one LGTM.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3585
+
   if (BaseOpcode->Store) { // No TFE for stores?
     Register VData = MI.getOperand(1).getReg();
----------------
Yes, store instructions don't support TFE to the best of my knowledge.

Store instructions can still be used on images that are partially resident, but they simply become no-ops if the destination address isn't mapped.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73446/new/

https://reviews.llvm.org/D73446