[PATCH] D59355: [RISCV] Optimize emission of SELECT sequences

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Added a few minor nits. You'll want to add a test for debug info to select-optimize-multiple.mir. I had a quick play with this and https://reviews.llvm.org/P8135 looks like a sensible way to do it.



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:911
+      // %Result = phi [ %TrueValue, HeadMBB ], [ %FalseValue, IfFalseMBB ]
+      BuildMI(*TailMBB, TailMBB->begin(), SelectMBBI->getDebugLoc(),
+              TII.get(RISCV::PHI), SelectMBBI->getOperand(0).getReg())
----------------
This inserts PHIs in the opposite order to the original selects. I'd maybe edit the comment at the head of the comment to say "The sequence of PHIs will be in the opposite order to the original selects, but this has no semantic impact".


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:876
+
+  // Transfer debug instructions regarding the selects to TailMBB.
+  for (MachineInstr *DebugInstr : SelectDebugValues) {
----------------
"associated with the selects" might be clearer phrasing


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:902
 
-  // %Result = phi [ %TrueValue, HeadMBB ], [ %FalseValue, IfFalseMBB ]
-  BuildMI(*TailMBB, TailMBB->begin(), DL, TII.get(RISCV::PHI),
-          MI.getOperand(0).getReg())
-      .addReg(MI.getOperand(4).getReg())
-      .addMBB(HeadMBB)
-      .addReg(MI.getOperand(5).getReg())
-      .addMBB(IfFalseMBB);
+  // Create PHIs for all of the select pseudo-instructions that were inserted.
+  auto SelectMBBI = MI.getIterator();
----------------
The creation of these phi nodes is part of the pseudo select "insertion" process. Perhaps "Create PHIs for the select pseudo-instructions in order to finish the custom insertion." Or really "Create PHIs for all of the select pseudo-instructions." would do.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59355