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
