On January 10, 2019 4:36:35 PM GMT+01:00, Eric Botcazou <ebotca...@adacore.com> wrote: >> Another problem is that in way too many cases we decide to choose >> BIGGEST_ALIGNMENT for stack slots, even when not strictly needed. >E.g. any >> BLKmode stack slot requests that BIGGEST_ALIGNMENT, even if >TYPE_ALIGN is >> much smaller and assign_stack_local_1 even asserts that for BLKmode >the >> alignment must be BIGGEST_ALIGNMENT. E.g. on mingw with -mavx or >-mavx512f >> BIGGEST_ALIGNMENT is 256 or 512 bits, but >MAX_SUPPORTED_STACK_ALIGNMENT is >> 128 bits. So the PR84877 change, even if it didn't cause wrong-code, >causes >> huge amounts of stack slots to be unnecessarily overaligned with >dynamic >> realignment. > >Yes, I think that we need to make sure that we dynamically realign only >when >this is necessary and... > >> The following patch reverts to the previous behavior and moves this >dynamic >> stack realignment to the caller that needs it, which doesn't do >caching and >> where we can do it solely for this overaligned DECL_ALIGN. > >...this modified implementation looks much safer than the original one. > >> If there are other spots that need this, wondering about: >> else >> copy = assign_temp (type, 1, 0); >> in calls.c, either it can be done by using the variable-sized object >> case in the then block, or could be done using assign_stack_local + >> this short realignment too. > >The latter I'd say. > >> Bootstrapped/regtested on x86_64-linux, i686-linux, >powerpc64le-linux, >> bootstrapped on powerpc64-linux (regtest still pending there). >> >> Ok for trunk? >> >> 2019-01-09 Jakub Jelinek <ja...@redhat.com> >> >> PR middle-end/84877 >> PR bootstrap/88450 >> * function.c (assign_stack_local_1): Revert the 2018-11-21 changes. >> (assign_parm_setup_block): Do the argument slot realignment here >> instead. > >FWIW this looks OK to me.
Given that, OK.. Richard.
