jrtc27 added inline comments.
================ 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 ---------------- MaskRay wrote: > 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. > Testing more variants is not wasteful if you can unify the compressed checking RUN lines below with these ones. There would only be 4 separate RUN lines in total, one for each, checking the assembly output (without aliases). ================ 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 ---------------- MaskRay wrote: > 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. Then use -riscv-no-aliases for the assembly-printing llc above, you don't need to go through assembling and dumping 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
