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

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 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,
----------------
asb wrote:
> 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."
Remove the comments for the override function.


================
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 \
----------------
asb wrote:
> 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.
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.


================
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:
----------------
asb wrote:
> 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.
Remove the attributes of test case IR.


Repository:
  rL LLVM

https://reviews.llvm.org/D42675