This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Use OS-specific stack-guard ABI for Fuchsia
ClosedPublic

Authored by mcgrathr on Feb 5 2023, 3:14 PM.

Details

Summary

Fuchsia provides a slot relative to tp for the stack-guard value,
which is cheaper to materialize than the default GOT load.

Diff Detail

Event Timeline

mcgrathr created this revision.Feb 5 2023, 3:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2023, 3:14 PM
mcgrathr requested review of this revision.Feb 5 2023, 3:14 PM
phosek accepted this revision.Feb 5 2023, 4:10 PM

LGTM

llvm/lib/Target/RISCV/RISCVSubtarget.h
166

Nit: We usually don't repeat the name of the field in the documentation comment (AArch64Subtarget.h does that but it's not common across LLVM).

This revision is now accepted and ready to land.Feb 5 2023, 4:10 PM
jrtc27 added inline comments.Feb 5 2023, 4:31 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
14224

Just inline this, it has a single use. Negating the condition in getIRStackGuard will help avoid nesting, too.

llvm/lib/Target/RISCV/RISCVSubtarget.h
167

This now duplicates XLen... besides, don't we get this already from MCSubtargetInfo?

180

This seems unnecessary

llvm/test/CodeGen/RISCV/stack-protector-target.ll
2

Use update_llc_test_checks.py, and use ;; for non-lit/FileCheck comments

4

Only one prefix so don't use --check-prefix*es*, and if this is the only triple tested then you don't need a custom prefix anyway

Also the -o - is redundant when compiling stdin

6

C++ mangling needlessly obfuscates the test

7–9

Shorter

mcgrathr marked 5 inline comments as done.Feb 5 2023, 6:11 PM
mcgrathr added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
14224

It will be reused when the safe-stack ABI support is added for Fuchsia, and perhaps also later if other targets choose tp-offset ABIs for these things as they have on other machines.

llvm/lib/Target/RISCV/RISCVSubtarget.h
180

I'm following the model of other targets that have various is{Target,OS}* shorthands. They become helpful as the number of checks using them increases.

llvm/test/CodeGen/RISCV/stack-protector-target.ll
4

I've left the explicit prefix in anticipation of other target-specific checks being added here later, but made the other simplifications.

mcgrathr updated this revision to Diff 494969.Feb 5 2023, 6:11 PM

cleanups per review

jrtc27 added inline comments.Feb 5 2023, 6:37 PM
llvm/test/CodeGen/RISCV/stack-protector-target.ll
11

nounwind will clean these up

28

I guess while you're fixing the above you can drop the nonnull, it doesn't do anything useful here

This revision was landed with ongoing or failed builds.Feb 5 2023, 6:46 PM
This revision was automatically updated to reflect the committed changes.
jrtc27 added a comment.Feb 5 2023, 6:58 PM

Latest feedback was not addressed when committing, and you gave hardly any time for anyone else who regularly works on or reviews changes for the RISC-V backend to comment

Latest feedback was not addressed when committing, and you gave hardly any time for anyone else who regularly works on or reviews changes for the RISC-V backend to comment

Sorry about that, I thought I'd done just as asked and hadn't seen the follow-up. https://reviews.llvm.org/D143360 has the recommended cleanups.

Thanks for the quick and thorough reviews. I'll wait a day for more comments on the cleanup change.