[PATCH] D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:154
+  MachineBasicBlock *Copy0MBB = F->CreateMachineBasicBlock(LLVM_BB);
+  MachineBasicBlock *Copy1MBB = F->CreateMachineBasicBlock(LLVM_BB);
+
----------------
A better name here would be more clear, maybe FallthroughMBB?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:171
+  int CC = MI.getOperand(3).getImm();
+  unsigned Opcode = -1;
+  switch (CC) {
----------------
Extracting this switch out as a helper function or lambda would aid readability.  getBranchOpcodeForSetCCOpcode?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:203
+  //  # fallthrough to Copy1MBB
+  BB = Copy0MBB;
+
----------------
Yuck.  Please just use the names and don't update a variable in place.  This is just confusing.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:205
+
+  // Update machine-CFG edges
+  BB->addSuccessor(Copy1MBB);
----------------
Didn't you already do this above?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:209
+  // Copy1MBB:
+  //  %Result = phi [ %FalseValue, Copy0MBB ], [ %TrueValue, ThisMBB ]
+  // ...
----------------
Hm, as written, isn't this basically making a static prediction the true value is common?  Is there profiling available on the SETCC instruction?


================
Comment at: test/CodeGen/RISCV/select-cc.ll:5
+; CHECK-LABEL: foo:
+; CHECK: beq a0, a2, .LBB{{.+}}
+  %val1 = load volatile i32, i32* %b
----------------
Please expand your test to cover the whole diamond.


https://reviews.llvm.org/D29937