[PATCH] D72796: AMDGPU/GlobalISel: Legalize unpacked d16 image operations

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

This seems largely reasonable to me, but I do think the observer notification needs to happen.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2731-2734
+  const AMDGPU::ImageDimIntrinsicInfo *ImageDimIntr =
+    AMDGPU::getImageDimIntrinsicInfo(IID);
+  if (!ImageDimIntr)
+    return true; // Other intrinsic to ignore
----------------
It might help the readability of legalizeIntrinsic to move this test there and pass the ImageDimIntr in?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2754
+
+    // FIXME: Do we need to notify the observer of the instruction change?
+    MI.getOperand(1).setReg(handleD16VData(B, *MRI, VData));
----------------
I think so?


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

https://reviews.llvm.org/D72796