[PATCH] D98610: [RISCV] Support clang -fpatchable-function-entry && GNU function attribute 'patchable_function_entry'

[Jessica Clarke via Phabricator via llvm-commits]

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", at 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