[PATCH] D118351: [AMDGPU][GlobalISel] Code quality: remove unnecessary or, and, shift instructions

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:334-335
+/// In case X and Y are independant, the combiner will do nothing.
+/// If X or Y is G_IMPLICIT_DEF, the result will be the other non-implicit
+/// register. The result of the previous expression is similar, but
+/// instead of having the same value as X or Y, it only holds the higher
----------------
This combine feels way too targeted. We shouldn't have to have combines that look for undefs in the operands of other things. Those operations on undef should have folded out on their own


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:345-346
+          SrcReg, MRI,
+          m_GBitcast(m_GOr(m_GAnd(m_MInstr(AndSrcMI), m_SpecificICst(0xffff)),
+                           m_GShl(m_MInstr(ShlSrcMI), m_SpecificICst(16)))))) {
+    if (ShlSrcMI->getOpcode() == TargetOpcode::G_IMPLICIT_DEF &&
----------------
Shouldn't hardcode these specific values, should compute based on the type bitwidth


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:351
+      return true;
+    } else if (AndSrcMI->getOpcode() == TargetOpcode::G_IMPLICIT_DEF &&
+             ShlSrcMI->getOpcode() == TargetOpcode::G_BITCAST) {
----------------
No else after return


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:355
+      return true;
+    } else if (ShlSrcMI->getOpcode() == TargetOpcode::G_LSHR &&
+               ShlSrcMI->getOperand(1).getReg() ==
----------------
No else after return


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:362
+      if (ShlCstArg->getOpcode() == TargetOpcode::G_CONSTANT &&
+          ShlCstArg->getOperand(1).isCImm() &&
+          ShlCstArg->getOperand(1).getCImm()->equalsInt(16)) {
----------------
Don't need this check


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

https://reviews.llvm.org/D118351