excessive stack usage compiling linux amdgpu kernel driver

[arndb]

Bugzilla Link	42551
Version	trunk
OS	Linux
Blocks	#4440
Attachments	preprocessed and partially reduced file, original source file, preprocessed and compressed, kernel patch to avoid passing structures by value
CC	@kbeyls,@nickdesaulniers,@ostannard,@zygoloid,@smithp35

Extended Description

One file in the linux kernel appears to trigger a failed optimization that leads to large stack usage. Compiling a 32-bit ARM defconfig with the amdgpu driver enabled, I get

drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dce_calcs.c:2987:6: error: stack frame size of 1344 bytes in function 'bw_calcs' [-Werror,-Wframe-larger-than=]
bool bw_calcs(struct dc_context *ctx,
^
drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dce_calcs.c:76:13: error: stack frame size of 5328 bytes in function 'calculate_bandwidth' [-Werror,-Wframe-larger-than=]

I managed to partially reduce the preprocessed source file to illustrate the problem better:

https://godbolt.org/z/ZlDp0Z

The stack usage is highly target architecture dependent:

gcc arm-linux-gnueabi: 208 bytes
clang-8 arm-linux-gnueabi: 4144 bytes
clang-9 arm-linux-gnueabi: 4992 bytes
clang-9 aarch64-linux-gnu: 272 bytes
clang-9 powerpc64: 4272 bytes
clang-9 powerpc32: 4112 bytes
clang-9 s390-32: 4168 bytes
clang-9 s390-64: 4168 bytes
clang-9 sparc32: 10272 bytes
clang-9 sparc64: 432 bytes

$ clang-9 -Wframe-larger-than=10 --target=arm-linux-gnu -O2 -fno-strict-overflow -S dce-calcs-clang-noinline.i -m32 -Wno-unused-value -Wno-logical-op-parentheses -Wno-return-type -Wno-implicit-int
dce-calcs-clang.i:275:4: warning: stack frame size of 4944 bytes in function 'calculate_bandwidth' [-Wframe-larger-than=]
calculate_bandwidth(struct bw_calcs_dceip *dceip, struct bw_calcs_vbios *vbios, struct bw_calcs_data *data) {
^

Note that we build 32-bit kernels with -Wframe-larger-than=1024 because of the highly limited available stack space, and using 5KB of stack is likely to result in a kernel crash.

[arndb]

Got a better reduced test case, see https://godbolt.org/z/z5bVKS

struct bw_fixed { long long value; };

struct bw_fixed bw_min2(struct bw_fixed, struct bw_fixed);
struct bw_fixed bw_max2(struct bw_fixed, struct bw_fixed);
struct bw_fixed bw_mul(struct bw_fixed, struct bw_fixed);

static inline struct bw_fixed bw_max3(struct bw_fixed v1, struct bw_fixed v2, struct bw_fixed v3) {
    return bw_max2(bw_max2(v1, v2), v3);
}

struct bw_fixed bw_int_to_fixed(long long value);

int f(struct bw_fixed _a, struct bw_fixed _b, struct bw_fixed _c)
{
    struct bw_fixed a=_a, b=_b, c=_c;
    struct bw_fixed a1=_a, b1=_b, c1=_c;
    struct bw_fixed a2=_a, b2=_b, c2=_c;

    a1 = bw_mul(bw_int_to_fixed(a.value), bw_int_to_fixed(3));
    b1 = bw_max3(a1, bw_mul(a2, a1), bw_mul(b2, c1));
    c1 = bw_max3(a2, bw_int_to_fixed(3), bw_int_to_fixed(2));

    return bw_max3(a1, b1, c1).value;
}

This just combines a couple of random operations from the original
kernel file. What can be seen here is that clang on ARM allocates
a stack slot for each temporary value resulting from calling
bw_int_to_fixed() or bw_max(), while it doesn't do that on x86-64,
and gcc never does it.

[lag-google]

Seeing this now too.

It's one of the last pieces in the puzzle in the context of clean CONFIG_WERROR builds with Clang.

Please could someone prioritise this?

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

[jayfoad]

I don't think this has anything to do with the AMDGPU backend for LLVM does it?

[lag-google]

No it doesn't. This is to do with the AMDGPU kernel driver.

[lag-google]

Please could someone add the clang label back?

[arndb]

FWIW, I confirmed that the problem happens with clang-16, pretty much unchanged from clang-9.

[ms178]

Similar excessive stack usage issues are also tracked in the ClangBuiltLinux-project with additional data points:

for x86_64: ClangBuiltLinux/linux#1754

and ARM64: ClangBuiltLinux/linux#1752

[arndb]

I'm fairly sure the other two issues are unrelated. The amdgpu display support runs into a lot of corner cases, this one happens specifically when passing around structs of 'long long' by value, and is reproducible exactly on the architectures that are triggered by the trivial test case above, the other ones appear to be caused by other problems in the driver source.

[ms178]

I've updated the wording of my comment accordingly, but as my entry still fits the rather bug headline which speaks about excessive stack usage in the amdgpu kernel driver in general, it might provide some value to keep the links here, no? If not, I have no problem with deleting my comments in this thread.

[jayfoad]

It seems to me that stack slots for sret arguments are not being shared because they don't have accurate lifetime markers when we get to the LLVM StackColoring pass.

This patch helps. Unfortunately it got reverted. https://reviews.llvm.org/D74094 "[IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas"

[nickdesaulniers]

It seems to me that stack slots for sret arguments are not being shared because they don't have accurate lifetime markers when we get to the LLVM StackColoring pass.

This patch helps. Unfortunately it got reverted. https://reviews.llvm.org/D74094 "[IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas"

I tested @arndb 's case from above back at bfaf535 (the commit before fafc6e4 got reverted (e26c24b).

It helped the --target=arm-linux-gnueabi case drop from 120B to 96B, and did nothing for x86. So it's not a complete solution, but perhaps a step in the right direction.

Also, I'm not sure how active the author of that patch is anymore. :-/

cc @nathanchance

[nickdesaulniers]

Testing @arndb 's initial report though: https://godbolt.org/z/ZlDp0Z
with https://reviews.llvm.org/D74094 revived+rebased:

--target=aarch64-linux-gnu:
208B -> 48B

--target=--target=arm-linux-gnueabi:
192B -> 40B

[nickdesaulniers]

https://reviews.llvm.org/rGe698695fbbf62e6676f8907665187f2d2c4d814b has been relanded. Closing. I will look at walking back commit 6f6cb1714365 ("drm/amdgpu: temporarily disable broken Clang builds due to blown stack-frame") for clang-18+.

[EugeneZelenko]

@nickdesaulniers: main will became 18.x eventually. You need only Git hash to refer to particular commit, without https://reviews.llvm.org/rG prefix.

[nickdesaulniers]

@EugeneZelenko I tagged this to the 18.0.0 release because this is fixed in clang-18.

[EugeneZelenko]

Tagging is used for backport from main to release branch or for issues that still need to be fixed in release branch. There is no 18 branch yet, so tag is useless.

[nickdesaulniers]

reverted (But I don't plan to reopen all of the bugs with the same root cause): #43598 (comment)