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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVMCInstLower.cpp:247
   }
+  return false;
 }
----------------
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`).


================
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:
> 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.



================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98610/new/

https://reviews.llvm.org/D98610