Fuchsia provides a slot relative to tp for the stack-guard value,
which is cheaper to materialize than the default GOT load.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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). |
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 |
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. |
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.
Just inline this, it has a single use. Negating the condition in getIRStackGuard will help avoid nesting, too.