nickdesaulniers added a comment.
In D104342#2820811 <https://reviews.llvm.org/D104342#2820811>, @dblaikie wrote:
> what're the rules about module level attributes like this? For instance: Does
> this affect/hinder inlining (if the attributes don't match between the caller
> and callee)? Other situations that might matter?
I think you meant s/module level/function level/? That's a good question, one
I had to think about a little bit. Here's my thoughts on the behavior this
should exhibit, please let me know if you agree.
When using `-Wframe-larger-than=<threshold>` per TU, the developer wants to be
alerted if any stack frame exceeds a threshold. The Linux kernel's use case is
that the kernel's stack is limited to (usually) two pages (`ulimit -s`;
typically 8KiB, but different architectures do support non-4KiB page sizes), so
functions using more than 1KiB of stack are usually indicative of large objects
being stack allocated that should have been heap allocated.
Currently, in C (and C with GNU extensions), there is no way to describe to the
compiler function-grain specific values for `-Wframe-larger-than=`; rather than
fine grain per function control, we only have coarse grain TU control.
So in the general case (non-LTO), we can only perform inline substitution at
call sites visible from callers' definitions. Because there's no GNU C function
attribute to change the current value of `-Wframe-larger-than=`, it's not
possible for that value to differ between caller and callee. But with LTO;
shit gets weird.
Suddenly now with LTO, we have cross TU (cross Module) visibility into call
sites, so we can inline across TU/Module boundaries. Thus we can have an IR
intermediary object with a call site where the caller's value of
`-Wframe-larger-than=` differs from the callees! So the question is what
should happen in such a case?
The extremely conservative approach which we have done in the past for certain
mismatched function attributes is to simply not perform inline substitution, if
we have no other options. This adds overhead to the inliner to check the XOR
of attribute lists of the caller and callee for each call site.
But I *think* (and am open to sugguestions) that we should:
1. permit inline substitution
2. the caller's value of `"warn-stack-size"=` IR Fn Attr wins
I think this is ok because: if caller is defined in TU1 with
`-Wframe-larger-than=` distinct from callee defined in TU2 with a different
value of `-Wframe-larger-than=`, then we don't care what callee's value was.
callee may even be DCE'd if it's inlined into a lone call site. I'd expect in
such cases that callee's value was larger than caller's, in which case callee
should be attributed `no_inline` for LTO if the tighter threshold for caller
now warns. If callee's value was smaller than callers and we performed inline
substitution, I think that's also perfectly fine, caller should not become
"more strict."
Generally in the Linux kernel, we see a common value of `-Wframe-larger-than=`
throughout most of the TUs, with only a few generally having a larger value to
relax constraints a little. (Also, those relaxations are questionable, given
the intent of `-Wframe-larger-than=` use in the kernel in the first place).
Let me add such a test to encode that intention; though I don't know yet what's
involved/possible to implement. Let's see.
================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:282
+ .getValueAsString()
+ .getAsInteger(0, Threshold);
if (StackSize > Threshold) {
----------------
dblaikie wrote:
> I guess the 0 value here is the default value if the value can't be parsed as
> an integer? Is that desirable? I guess maybe we should ignore it (use
> UINT_MAX here instead, maybe) and fail in the verifier.
>
> But I guess if we fail in the verifier, then it doesn't really
> matter/shouldn't be tested what the behavior is here when presented with
> invalid IR.
>
> (but this is a divergence from the module flag handling, which looks like it
> does silently ignore non-numeric values, by using UINT_MAX)
IIUC, the first parameter to `getAsInteger` is the `Radix`, not the default
value on failure to parse. But it does return `true` on error, so I should
check that here.
I also should add a verifier check for this new function attribute. While the
"string key equals string value" attributes are quite flexible, it would be
good to have some rigidity in requiring the string value to be parseable as an
unsigned int.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104342/new/
https://reviews.llvm.org/D104342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
