[PATCH] D90217: [AMDGPU][GlobalISel] Fold a chain of two shift instructions with constant operands

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1551-1553
+  if (Opcode != TargetOpcode::G_SHL && Opcode != TargetOpcode::G_ASHR &&
+      Opcode != TargetOpcode::G_LSHR)
+    return false;
----------------
Change to assert().


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1562
+  MachineInstr *Shl2Def = MRI.getUniqueVRegDef(Shl2);
+  if (!Shl2Def || Shl2Def->getOpcode() != Opcode)
+    return false;
----------------
I don't think Shl2Def can ever be null here, can it?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1588
+
+  if (MatchInfo.Imm >= ImmTy.getScalarSizeInBits()) {
+    // Any logical shift that exceeds scalar size will produce zero.
----------------
"Imm" instead of "MatchInfo.Imm"


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1588
+
+  if (MatchInfo.Imm >= ImmTy.getScalarSizeInBits()) {
+    // Any logical shift that exceeds scalar size will produce zero.
----------------
foad wrote:
> "Imm" instead of "MatchInfo.Imm"
Logically you want the size of the first operand here, not the size of the immediate. I'm not sure if they are guaranteed to be the same.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1595
+    }
+    // Arithmetic shift larger then scalar size has no effect.
+    Imm = ImmTy.getScalarSizeInBits() - 1;
----------------
Typo "than".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90217