[PATCH] D151449: [RISCV] Add DAG combine for CTTZ/CTLZ in the case of input 0

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11427
+
+  // This represents eihter CTTZ or CTLZ instruction.
+  SDValue CTZ;
----------------
eiher -> either


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11429
+  SDValue CTZ;
+
+  SDValue ValOnZero;
----------------
mgudim wrote:
> Suggestion: I would use `Count` or `CountZeroes`. On ARM `CTZ` means `CTTZ`.
I agree. ctz is also the trailing zero instruction on RISC-V.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11449
+
+  if (CTZ.getOpcode() == ISD::TRUNCATE || CTZ.getOpcode() == ISD::ZERO_EXTEND)
+    CTZ = CTZ.getOperand(0);
----------------
Do we need to guard against truncating to a type that can't represent the entire bitwidth. Or does that just workout because the AND we create would be all ones in that case?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11459
+
+  SDValue LHS = Cond->getOperand(0);
+  SDValue CTZArgument = CTZ->getOperand(0);
----------------
No need for `LHS` just put `Cond->getOperand(0)` into the `if`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11465
+
+  SDValue CTZZeroDef;
+  switch (CTZ.getOpcode()) {
----------------
Do we need a new variable? Can we assign over CTZ for the _UNDEF cases?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11490
+
+  if (CTZZeroDef.getValueType() == N->getValueType(0))
+    return AndNode;
----------------
This if is unnecessary, getZExtOrtrunc will take care of it.


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

https://reviews.llvm.org/D151449