[PATCH] D29937: [RISCV 15/n] Implement lowering of ISD::SELECT_CC
Philip Reames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 24 17:24:22 PDT 2017
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
More information about the llvm-commits
mailing list