[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/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