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

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:173
+  // -> (riscvisd::select_cc condv, zero, setne, truev, falsev)
+  SDValue Zero = DAG.getConstant(0, DL, XLenVT);
+  SDValue SetNE = DAG.getConstant(ISD::SETNE, DL, XLenVT);
----------------
johnrusso wrote:
> I noticed that the select-cc.ll test didn't cover this code. 
> This little snip with the correct matching pattern should cover it.
> ...
>   %val20 = select i1 %tst10, i32 %val18, i32 %val19
> 
>   %tr3 = trunc i32 %val3 to i1
>   %val21 = select i1 %tr3, i32 %val20, i32 %val19
> 
>   ret i32 %val21
> 
Well spotted, I've moved the bare-select.ll test from D29938 in to this patch to ensure the code path is exercised.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:188
+
+  assert(MI.getOpcode() == RISCV::Select_GPR_Using_CC_GPR &&
+         "Unexpected instr type to insert");
----------------
johnrusso wrote:
> Could we use a 'switch(MI.getOpcode())' here and assert on an unhandled opcode if it occurs. I suspect other pseudos may require a custom inserter in the future.
Future patches [move to doing this](https://github.com/lowRISC/riscv-llvm/blob/a24e5170c59eaf8cf7423dcda824c1770868e24d/0056-RISCV-Codegen-support-for-floating-point-comparison-.patch). For previous patches, reviewers have asked to not have single-element switches. I don't have a particularly strong view either way. The single-element switch minimises the diff in the future patches and is easier to extend, but has the disadvantage of looking a little odd.


https://reviews.llvm.org/D29937