[PATCH] D39912: AMDGPU/SI: Implement d16 support for image intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:549
 
+static unsigned getImageOpcode (unsigned IID) {
+  switch (IID) {
----------------
Extra space before (, can use Intrinsic::ID type


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:551
+  switch (IID) {
+    case Intrinsic::amdgcn_image_load : return AMDGPUISD::IMAGE_LOAD;
+    case Intrinsic::amdgcn_image_load_mip : return AMDGPUISD::IMAGE_LOAD_MIP;
----------------
Returns on separte line


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4906-4908
+    SDValue VData = Op.getOperand(2);
+    EVT StoreVT = VData.getValueType();
+    if (isHalfVT(StoreVT)) {
----------------
Can't you reuse the type convert function from the buffer patch?


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7125
+    //int D16Idx = AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::d16);
+    //if (D16Idx > 0 && MI.getOperand(D16Idx).getImm() != 0)
+    if (TII->isD16(MI))
----------------
Dead code. Comment isn't helpful, this is a TODO to implement this optimization for d16. Can you open a bug for that?.

For the unpacked case I would expect it to be trivial to extend for it


https://reviews.llvm.org/D39912