[PATCH] D85887: [AMDGPU] Add A16/G16 to InstCombine

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:56
 
+// Check if a value can be converted to a 16-bit value without loosing
+// precision.
----------------
Typo loosing


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:63
+    return false;
+  } else if (ConstantFP *ConstFloat = dyn_cast<ConstantFP>(&V)) {
+    // We need to check that if we cast the index down to a half, we do not lose
----------------
No else after return


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:70
+    return !LosesInfo;
+  } else if (isa<FPExtInst>(&V) || isa<SExtInst>(&V) || isa<ZExtInst>(&V)) {
+    Value *CastSrc = cast<Instruction>(&V)->getOperand(0);
----------------
No else after return. Also use match()?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:85
+    return cast<Instruction>(&V)->getOperand(0);
+  else if (VTy->isIntegerTy())
+    return Builder.CreateIntCast(&V, Type::getInt16Ty(V.getContext()), false);
----------------
No else after return


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:91
+  llvm_unreachable("Should never be called!");
+  return nullptr;
+}
----------------
Dead code


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:96
+    const GCNSubtarget *ST,
+    const llvm::AMDGPU::ImageDimIntrinsicInfo *ImageDimIntr, IntrinsicInst &II,
+    InstCombiner &IC) {
----------------
Don't need llvm::


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:113
+        return None;
+      } else {
+        // All gradients can be converted, so convert only them
----------------
No else after return


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:822-824
+      const TargetMachine &TM = getTLI()->getTargetMachine();
+      const GCNSubtarget *ST = static_cast<const GCNSubtarget *>(
+          TM.getSubtargetImpl(*II.getFunction()));
----------------
The subtarget is already available in GCNTTI


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85887