jrtc27 added inline comments.
================ Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:53 +void RISCVInstrInfo::getNoop(MCInst &NopInst) const { + if (STI.getFeatureBits()[RISCV::FeatureStdExtC]) ---------------- MaskRay wrote: > jrtc27 wrote: > > I will forever wonder why TII didn't make it `MCInst getNoop()`... > Let me do a refactoring before this patch? Definitely won't complain if you're volunteering to fix it to be a saner interface :) ================ Comment at: llvm/lib/Target/RISCV/RISCVMCInstLower.cpp:247 } + return false; } ---------------- MaskRay wrote: > 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`). Ok, yes, I agree now, worked out what's going on ================ Comment at: llvm/test/CodeGen/RISCV/patchable-function-entry.ll:24 +; RVC-NEXT: c.jr ra +; CHECK: .section __patchable_function_entries,"awo",@progbits,f1{{$}} +; 32: .p2align 2 ---------------- Is there a benefit from treating these like labels with CHECK-LABEL? ================ 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: > 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). Ok now please use RV32/RV64 rather than bare 32/64 to be more consistent with existing tests 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