[PATCH] D127871: [RISCV] Optimize 2x SELECT for floating-point types

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9630
+  auto FirstCC = static_cast<RISCVCC::CondCode>(First.getOperand(3).getImm());
+  Register fLHS = First.getOperand(1).getReg();
+  Register fRHS = First.getOperand(2).getReg();
----------------
Variable names should start with a capital letter


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9637
+      .addMBB(SinkMBB);
+  BuildMI(ThisMBB, DL, TII.get(RISCV::PseudoBR)).addMBB(FirstMBB);
+
----------------
Why do we need an explicit PseudoBR? Can't we let it fallthrough?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9709
+  MachineBasicBlock::iterator NextMIIt = MachineBasicBlock::iterator(MI);
+  ++NextMIIt;
+  if (MI.getOpcode() != RISCV::Select_GPR_Using_CC_GPR &&
----------------
Should this use `next_nodbg` to increment?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9711
+  if (MI.getOpcode() != RISCV::Select_GPR_Using_CC_GPR &&
+      NextMIIt->getOpcode() == MI.getOpcode() && NextMIIt != BB->end() &&
+      NextMIIt->getOperand(5).getReg() == MI.getOperand(0).getReg() &&
----------------
Shouldn't we be checking `NextMIIt != BB->end()` before checking the opcode of `NextMIIt`?


================
Comment at: llvm/test/CodeGen/RISCV/select-optimize-multiple.ll:537
+
+define dso_local float @CascadedSelect(float noundef %a) local_unnamed_addr #0 {
+; RV32I-LABEL: CascadedSelect:
----------------
Drop dso_local and local_unnamed_addr and `#0`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127871