[RISCV] Define getSetCCResultType for setting vector setCC type

[shiva0217]

Summary

To avoid trigger "No default SetCC type for vectors!" assertion for vector comparison.

[asb]

lib/Target/RISCV/RISCVISelLowering.h

54 ↗

(On Diff #131913)

The LLVM Coding Standards doc isn't explicit in its guidance on doxygen comments about whether to duplicate them in subclasses. I'd generally prefer not to personally, but will ask on the mailing list for clarification on general policy. See question here.

If the comment is going to stay, the current LLVM style is to avoid duplicating the function name. I also think the version of the comment in include/llvm/CodeGen/TargetLowering.h is clearer "/// Return the ValueType of the result of SETCC operations."

test/CodeGen/RISCV/get-setcc-result-type.ll

1 ↗

(On Diff #131913)

The CHECK lines don't seem to have been generated by update_llc_test_checks.py.

Even though it's a little verbose, I'd recommend leaving the full check output from update_llc_test_checks.py. All other codegen tests do this, and I've found that overall the advantage of being able to trivially regenerate a test outweighs any disadvantage in terms of brittleness or verbose output. Besides, the subset of instructions currently checked in this patch aren't really enough for the casual reader to verify that the expected code is being produced.

5 ↗

(On Diff #131913)

I tend to prefer dropping any attributes that aren't necessary for the test - in this case define void @getSetCCResultType(<4 x i32>* %p, <4 x i32>* %q) results in identical codegen.

[shiva0217]

Update the patch to reflect the comments.

[shiva0217]

lib/Target/RISCV/RISCVISelLowering.h

54 ↗

(On Diff #131913)

Remove the comments for the override function.

test/CodeGen/RISCV/get-setcc-result-type.ll

1 ↗

(On Diff #131913)

Update the test case with full context assembly output.
Hi Alex. I'm not really familiar with update_llc_test_checks.py. Could you give me some guidance on why we should use it and what we should take care when we write a test case with it? Thanks.

5 ↗

(On Diff #131913)

Remove the attributes of test case IR.

[asb]

Hi Shiva. update_llc_test_checks.py basically generates CHECK lines for you according to the RUN lines in the file. This is a fairly minor benefit when writing a test for the first time, but the biggest benefit comes from maintaining the test. When there is a codegen change, test files that contain all instructions are much easier to review.

Previous workflow:

• Create and commit a test, manually write CHECK lines, typically picking out a subset of instructions to CHECK
• A change is later made that affects codegen. Typically you would manually inspect the previous and new output, then manually update CHECK lines

New work flow:

• Create a test, have update_llc_test_checks.py generate CHECK lines for you. Double-check they are as expected before committing
• A change is later made that affects codegen. I normally immediately use update_llc_test_checks.py to update a file, then use git diff to review the change for correctness.
• As an example, the use of update_llc_test_checks.py made it totally trivial to update all codegen tests when @niosHD introduced pseudoinstruction aliases

The risk of update_llc_test_checks.py is that by checking each instruction, tests are more brittle to codegen changes. I haven't found this to be an issue in practice, and flagging up even minor codegen differences can be an advantage. An increasing number of tests for other backends (especially x86) are moving over to doing this for the maintainability benefits.

Assuming your LLVM build dir is within your LLVM checkout (e.g. build), you can invoke update_llc_test_checks.py like: ../utils/update_llc_test_checks.py --llc-binary=./bin/llc ../test/CodeGen/RISCV/get-setcc-result-type.ll.

This looks good to commit to me, once the two trivial comments I've added on the test are addressed. Thanks!

test/CodeGen/RISCV/get-setcc-result-type.ll

2 ↗

(On Diff #132092)

-disable-block-placement has no codegen impact for this test, so remove the option.

7–28 ↗

(On Diff #132092)

Whitespace here doesn't match the output of update_llc_test_checks.py, meaning we'll get unwanted diffs the next time the test is regenerated. Use the invocation I explain in my other comment to regenerate.

[shiva0217]

Hi Alex. Thanks for your detail explanation, I really appreciate that. This is a great tool we should use. The commit version has reflected comments.

llvm/trunk/lib/Target/RISCV/RISCVISelLowering.h

public:
   std::pair<unsigned, const TargetRegisterClass *>
   getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                StringRef Constraint, MVT VT) const override;
 
   MachineBasicBlock *
   EmitInstrWithCustomInserter(MachineInstr &MI,
                               MachineBasicBlock *BB) const override;
 
+  EVT getSetCCResultType(const DataLayout &DL, LLVMContext &Context,
+                         EVT VT) const override;
+
 private:
   void analyzeInputArgs(MachineFunction &MF, CCState &CCInfo,
                         const SmallVectorImpl<ISD::InputArg> &Ins,
                         bool IsRet) const;
   void analyzeOutputArgs(MachineFunction &MF, CCState &CCInfo,
                          const SmallVectorImpl<ISD::OutputArg> &Outs,
                          bool IsRet, CallLoweringInfo *CLI) const;
   // Lower incoming arguments, copy physregs into vregs

llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp

RISCVTargetLowering::RISCVTargetLowering(const TargetMachine &TM,
   // Function alignments (log2).
   setMinFunctionAlignment(3);
   setPrefFunctionAlignment(3);
 
   // Effectively disable jump table generation.
   setMinimumJumpTableEntries(INT_MAX);
 }
 
+EVT RISCVTargetLowering::getSetCCResultType(const DataLayout &DL, LLVMContext &,
+                                            EVT VT) const {
+  if (!VT.isVector())
+    return getPointerTy(DL);
+  return VT.changeVectorElementTypeToInteger();
+}
+
 // Changes the condition code and swaps operands if necessary, so the SetCC
 // operation matches one of the comparisons supported directly in the RISC-V
 // ISA.
 static void normaliseSetCC(SDValue &LHS, SDValue &RHS, ISD::CondCode &CC) {
   switch (CC) {
   default:
     break;
   case ISD::SETGT:

llvm/trunk/test/CodeGen/RISCV/get-setcc-result-type.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck -check-prefix=RV32I %s
+
+define void @getSetCCResultType(<4 x i32>* %p, <4 x i32>* %q) {
+; RV32I-LABEL: getSetCCResultType:
+; RV32I:       # %bb.0: # %entry
+; RV32I-NEXT:    lw a1, 12(a0)
+; RV32I-NEXT:    xor a1, a1, zero
+; RV32I-NEXT:    seqz a1, a1
+; RV32I-NEXT:    neg a1, a1
+; RV32I-NEXT:    sw a1, 12(a0)
+; RV32I-NEXT:    lw a1, 8(a0)
+; RV32I-NEXT:    xor a1, a1, zero
+; RV32I-NEXT:    seqz a1, a1
+; RV32I-NEXT:    neg a1, a1
+; RV32I-NEXT:    sw a1, 8(a0)
+; RV32I-NEXT:    lw a1, 4(a0)
+; RV32I-NEXT:    xor a1, a1, zero
+; RV32I-NEXT:    seqz a1, a1
+; RV32I-NEXT:    neg a1, a1
+; RV32I-NEXT:    sw a1, 4(a0)
+; RV32I-NEXT:    lw a1, 0(a0)
+; RV32I-NEXT:    xor a1, a1, zero
+; RV32I-NEXT:    seqz a1, a1
+; RV32I-NEXT:    neg a1, a1
+; RV32I-NEXT:    sw a1, 0(a0)
+; RV32I-NEXT:    ret
+entry:
+  %0 = load <4 x i32>, <4 x i32>* %p, align 16
+  %cmp = icmp eq <4 x i32> %0, zeroinitializer
+  %sext = sext <4 x i1> %cmp to <4 x i32>
+  store <4 x i32> %sext, <4 x i32>* %p, align 16
+  ret void
+}