[PATCH] D104342: [IR] convert warn-stack-size from module attr to fn attr

Thu Jun 17 11:50:10 PDT 2021

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