[Openmp-commits] [openmp] [Libomptarget] Handle dynamic stack sizes for AMD COV5 (PR #72606)

[Jon Chesterfield via Openmp-commits]

JonChesterfield wrote:

It's really difficult to work out what's going on here from the diff.

I think you've rolled a change in default from 16k to 1k in along with the rest of it and got a branch inverted but it's hard to tell.

What we used to have is the compiler emitted a guess at the stack size needed. Now it just punts with "no idea, good luck runtime", in which case we assume 1k and fall over if that's no good? This behaviour is pretty weird - if the compiler can't tell how much stack to allocate, the chances that the single right number for all the difficult kernels is in an *environment variable* seems pretty remote.

I think we should go with more sensible semantics or be much clearer with the naming of of things here. Something like:

`bool kernel_metadata_missing_stack_size()`
`uint64_t guess_stack_size_for_missing_metadata_from_environment_variable()`

For more sensible semantics, we know the upper bound on how much stack size we could give a kernel, and we know that the compiler failing to determine a stack size means the application is doing something exciting with control flow, so I think we should (optionally warn and) default to the maximum. That is, your complicated kernel doing stuff we failed to analyse gets to run slowly, unless you annotate it somehow.

Further, this single environment variable to set all stack sizes for all unknown things is just a dreadful construct and I recommend should ignore it entirely.



https://github.com/llvm/llvm-project/pull/72606