MaskRay marked 2 inline comments as done. MaskRay added inline comments.
================ Comment at: llvm/lib/Target/RISCV/RISCVMCInstLower.cpp:247 } + return false; } ---------------- jrtc27 wrote: > true, surely? false. We want to call `EmitToStreamer(*OutStreamer, TmpInst);` If a pseudo instruction is not handled here, in MCAsmStreamer we will see its comment (`AsmString`). ================ Comment at: llvm/test/CodeGen/RISCV/patchable-function-entry.ll:1-3 +;; Test the function attribute "patchable-function-entry". +; RUN: llc -mtriple=riscv32 < %s | FileCheck %s --check-prefixes=CHECK,32 +; RUN: llc -mtriple=riscv64 < %s | FileCheck %s --check-prefixes=CHECK,64 ---------------- jrtc27 wrote: > luismarques wrote: > > jrtc27 wrote: > > > Please match the style of the other tests: > > > - update_llc_test_checks.py > > > - RV32I/RV32IC/RV64I/RV64IC > > > Please match the style of the other tests: > > > - update_llc_test_checks.py > > > > I was going to comment on that but I was unsure it applied here, given that > > this test was running more than just `llc`. The other RISC-V CodeGen tests > > we have with objdump don't use the update script IIRC. (And these manual > > tests were neatly written and much more condensed that the sprawling output > > of the auto updated checks). But I agree that in general using the script > > is the way to go for the RISC-V tests. > Yeah, though as you'll see in my comment below I don't think using > llvm-objdump adds any value, at which point UTC works just fine. I do not think we can use `update_llc_test_checks.py`. We should test `__patchable_function_entries` which can be scrubbed by `update_llc_test_checks.py`. The test is written in such a way with low maintenance: it will very unlikely change due to codegen differences. (The one `; CHECK-NEXT: addi sp, sp, -16` below is the only exception.) Testing more variants seems wasteful. ================ Comment at: llvm/test/CodeGen/RISCV/patchable-function-entry.ll:5-9 +;; RVC uses 2-byte nop. +; RUN: llc -filetype=obj -mtriple=riscv64 %s -o %t.o +; RUN: llvm-objdump -d %t.o | FileCheck %s --check-prefix=NORVC +; RUN: llc -filetype=obj -mtriple=riscv64 -mattr=+c %s -o %tc.o +; RUN: llvm-objdump -d %tc.o | FileCheck %s --check-prefix=RVC ---------------- jrtc27 wrote: > I don't think this is particularly useful if we've checked the assembly > output (and it stops us using update_llc_test_checks.py), we know nops get > emitted correctly, that's tested in various other places. objdump -d/llvm-objdump -d does not print c.nop => it is an important detail. Without it we cannot make sure the correct number of bytes is used. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98610/new/ https://reviews.llvm.org/D98610 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
