[PATCH] D117875: [AMDGPU][InstCombine] Use D16 if only f16 precision is needed

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:64-67
+  if (STy->isHalfTy() || STy->isIntegerTy(16)) {
     // The value is already 16-bit, so we don't want to convert to 16-bit again!
     return false;
   }
----------------
I don't understand why we need this code.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:90-91
   if (match(&V, m_FPExt(PatternMatch::m_Value(CastSrc))) ||
       match(&V, m_SExt(PatternMatch::m_Value(CastSrc))) ||
       match(&V, m_ZExt(PatternMatch::m_Value(CastSrc)))) {
+    Type *CastSrcTy = CastSrc->getType()->getScalarType();
----------------
I know you haven't changed this but it seems dangerous: for a d16 input treated as unsigned you should not match sext here, and for a d16 input treated as signed you should not match zext. Or do all d16 integer inputs ignore the high bits?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:107
+      VTy->isFPOrFPVectorTy() ? Builder.getHalfTy() : Builder.getInt16Ty();
+  Type *NewTy;
+  if (auto *VectorTy = dyn_cast<VectorType>(VTy))
----------------
`NewTy = VTy->getWithNewType(NewScalarTy)`?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstrInfo.h:41
 
-struct D16ImageDimIntrinsic {
-  unsigned Intr;
----------------
If this was already unused, you can remove it with a separate NFC patch (consider it pre-approved if you want).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117875