[PATCH] D42675: [RISCV] Define getSetCCResultType for setting vector setCC type

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.h:54
 
+  /// getSetCCResultType - Return the value type to use for ISD::SETCC.
+  EVT getSetCCResultType(const DataLayout &DL, LLVMContext &Context,
----------------
The LLVM Coding Standards doc isn't explicit in its [guidance on doxygen comments](https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-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](http://lists.llvm.org/pipermail/llvm-dev/2018-January/120882.html).

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."


================
Comment at: test/CodeGen/RISCV/get-setcc-result-type.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -disable-block-placement -verify-machineinstrs < %s \
----------------
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.


================
Comment at: test/CodeGen/RISCV/get-setcc-result-type.ll:5
+
+define void @getSetCCResultType(<4 x i32>* nocapture %p, <4 x i32>* nocapture readnone %q) local_unnamed_addr #0 {
+; RV32I-LABEL: getSetCCResultType:
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D42675