Page MenuHomePhabricator

[RISCV] Support clang -fpatchable-function-entry && GNU function attribute 'patchable_function_entry'
ClosedPublic

Authored by MaskRay on Mar 14 2021, 2:08 PM.

Details

Summary

Similar to D72215 (AArch64) and D72220 (x86).

% clang -target riscv32 -march=rv64g -c -fpatchable-function-entry=2 a.c && llvm-objdump -dr a.o
...
0000000000000000 <main>:
       0: 13 00 00 00   nop
       4: 13 00 00 00   nop

% clang -target riscv32 -march=rv64gc -c -fpatchable-function-entry=2 a.c && llvm-objdump -dr a.o
...
00000002 <main>:
       2: 01 00         nop
       4: 01 00         nop

Recently the mainline kernel started to use -fpatchable-function-entry=8 for riscv (https://git.kernel.org/linus/afc76b8b80112189b6f11e67e19cf58301944814).

Diff Detail

Event Timeline

MaskRay created this revision.Mar 14 2021, 2:08 PM
MaskRay requested review of this revision.Mar 14 2021, 2:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 14 2021, 2:08 PM
MaskRay edited the summary of this revision. (Show Details)Mar 14 2021, 2:11 PM
luismarques accepted this revision.Mar 14 2021, 3:19 PM

Overall LGTM.
I just don't understand what you mean with "1-byte NOPs" in the patchable prefix case. Regular NOPs are emitted. Please clarify the comment/patch as needed.

This revision is now accepted and ready to land.Mar 14 2021, 3:19 PM
MaskRay updated this revision to Diff 330548.Mar 14 2021, 6:27 PM

fix comment

jrtc27 added inline comments.Mar 14 2021, 6:41 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
53

I will forever wonder why TII didn't make it MCInst getNoop()...

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp
247

true, surely?

llvm/test/CodeGen/RISCV/patchable-function-entry.ll
2–4

Please match the style of the other tests:

  • update_llc_test_checks.py
  • RV32I/RV32IC/RV64I/RV64IC
6–10

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.

luismarques added inline comments.Mar 14 2021, 7:08 PM
llvm/test/CodeGen/RISCV/patchable-function-entry.ll
2–4

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.

The attribute bits look fine to me aside from the documentation request.

clang/include/clang/Basic/Attr.td
738

We should probably add some documentation about what targets this attribute is supported on.

jrtc27 added inline comments.Mar 15 2021, 9:45 AM
llvm/test/CodeGen/RISCV/patchable-function-entry.ll
2–4

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.

MaskRay marked 2 inline comments as done.Mar 15 2021, 11:03 AM
MaskRay added inline comments.
llvm/lib/Target/RISCV/RISCVMCInstLower.cpp
247

false.

We want to call EmitToStreamer(*OutStreamer, TmpInst);

If a pseudo instruction is not handled here, in MCAsmStreamer we will see its comment (AsmString).

llvm/test/CodeGen/RISCV/patchable-function-entry.ll
2–4

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.

6–10

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.

jrtc27 added inline comments.Mar 15 2021, 11:11 AM
llvm/test/CodeGen/RISCV/patchable-function-entry.ll
2–4

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

6–10

Then use -riscv-no-aliases for the assembly-printing llc above, you don't need to go through assembling and dumping

MaskRay updated this revision to Diff 330744.Mar 15 2021, 11:32 AM
MaskRay marked 8 inline comments as done.

Mention supported targets in clang/include/clang/Basic/AttrDocs.td

Use 4 llc RUN lines and --riscv-no-aliases

MaskRay added inline comments.Mar 15 2021, 11:34 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
53

Let me do a refactoring before this patch?

jrtc27 added inline comments.Mar 15 2021, 11:39 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
53

Definitely won't complain if you're volunteering to fix it to be a saner interface :)

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp
247

Ok, yes, I agree now, worked out what's going on

llvm/test/CodeGen/RISCV/patchable-function-entry.ll
2–4

Ok now please use RV32/RV64 rather than bare 32/64 to be more consistent with existing tests

25

Is there a benefit from treating these like labels with CHECK-LABEL?

MaskRay updated this revision to Diff 330759.Mar 15 2021, 12:12 PM
MaskRay marked 5 inline comments as done.

Change check prefixes from 32/64 to RV32/RV64

Rebase and use MCInst getNop

llvm/test/CodeGen/RISCV/patchable-function-entry.ll
25

I don't find a difference regarding the diagnostic.

The <stdin>:29:2: note: possible intended match here\n .section __patchable_function_entries,"awo",@progbits,f1 lines do not change. So -LABEL: does not improve diagnostic.

jrtc27 accepted this revision.Mar 15 2021, 3:44 PM
This revision was landed with ongoing or failed builds.Mar 16 2021, 10:02 AM
This revision was automatically updated to reflect the committed changes.