On Mon, Jul 10, 2023 at 3:32 AM Richard Biener <richard.guent...@gmail.com> wrote: > > On Fri, Jul 7, 2023 at 5:14 PM H.J. Lu via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > Don't assume that stack slots can only be accessed by stack or frame > > registers. We first find all registers defined by stack or frame > > registers. Then check memory accesses by such registers, including > > stack and frame registers. > > > > gcc/ > > > > PR target/109780 > > * config/i386/i386.cc (ix86_update_stack_alignment): New. > > (ix86_find_all_reg_use): Likewise. > > (ix86_find_max_used_stack_alignment): Also check memory accesses > > from registers defined by stack or frame registers. > > > > gcc/testsuite/ > > > > PR target/109780 > > * g++.target/i386/pr109780-1.C: New test. > > * gcc.target/i386/pr109780-1.c: Likewise. > > * gcc.target/i386/pr109780-2.c: Likewise. > > --- > > gcc/config/i386/i386.cc | 120 +++++++++++++++++---- > > gcc/testsuite/g++.target/i386/pr109780-1.C | 72 +++++++++++++ > > gcc/testsuite/gcc.target/i386/pr109780-1.c | 14 +++ > > gcc/testsuite/gcc.target/i386/pr109780-2.c | 21 ++++ > > 4 files changed, 206 insertions(+), 21 deletions(-) > > create mode 100644 gcc/testsuite/g++.target/i386/pr109780-1.C > > create mode 100644 gcc/testsuite/gcc.target/i386/pr109780-1.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr109780-2.c > > > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > > index caca74d6dec..27f349b0ccb 100644 > > --- a/gcc/config/i386/i386.cc > > +++ b/gcc/config/i386/i386.cc > > @@ -8084,6 +8084,63 @@ output_probe_stack_range (rtx reg, rtx end) > > return ""; > > } > > > > +/* Update the maximum stack slot alignment from memory alignment in > > + PAT. */ > > + > > +static void > > +ix86_update_stack_alignment (rtx, const_rtx pat, void *data) > > +{ > > + /* This insn may reference stack slot. Update the maximum stack slot > > + alignment. */ > > + subrtx_iterator::array_type array; > > + FOR_EACH_SUBRTX (iter, array, pat, ALL) > > + if (MEM_P (*iter)) > > + { > > + unsigned int alignment = MEM_ALIGN (*iter); > > + unsigned int *stack_alignment > > + = (unsigned int *) data; > > + if (alignment > *stack_alignment) > > + *stack_alignment = alignment; > > + break; > > + } > > +} > > + > > +/* Find all registers defined with REG. */ > > + > > +static void > > +ix86_find_all_reg_use (HARD_REG_SET &stack_slot_access, int reg) > > +{ > > + for (df_ref ref = DF_REG_USE_CHAIN (reg); > > + ref != NULL; > > + ref = DF_REF_NEXT_REG (ref)) > > + { > > + if (DF_REF_IS_ARTIFICIAL (ref)) > > + continue; > > + > > + rtx_insn *insn = DF_REF_INSN (ref); > > + if (!NONDEBUG_INSN_P (insn)) > > + continue; > > + > > + rtx set = single_set (insn); > > + if (!set) > > + continue; > > + > > + rtx src = SET_SRC (set); > > + if (MEM_P (src)) > > + continue; > > + > > + rtx dest = SET_DEST (set); > > + if (!REG_P (dest)) > > + continue; > > + > > + if (TEST_HARD_REG_BIT (stack_slot_access, REGNO (dest))) > > + continue; > > + > > + /* Add this register to stack_slot_access. */ > > + add_to_hard_reg_set (&stack_slot_access, Pmode, REGNO (dest)); > > + } > > +} > > + > > /* Set stack_frame_required to false if stack frame isn't required. > > Update STACK_ALIGNMENT to the largest alignment, in bits, of stack > > slot used if stack frame is required and CHECK_STACK_SLOT is true. */ > > @@ -8102,10 +8159,6 @@ ix86_find_max_used_stack_alignment (unsigned int > > &stack_alignment, > > add_to_hard_reg_set (&set_up_by_prologue, Pmode, > > HARD_FRAME_POINTER_REGNUM); > > > > - /* The preferred stack alignment is the minimum stack alignment. */ > > - if (stack_alignment > crtl->preferred_stack_boundary) > > - stack_alignment = crtl->preferred_stack_boundary; > > - > > bool require_stack_frame = false; > > > > FOR_EACH_BB_FN (bb, cfun) > > @@ -8117,27 +8170,52 @@ ix86_find_max_used_stack_alignment (unsigned int > > &stack_alignment, > > set_up_by_prologue)) > > { > > require_stack_frame = true; > > - > > - if (check_stack_slot) > > - { > > - /* Find the maximum stack alignment. */ > > - subrtx_iterator::array_type array; > > - FOR_EACH_SUBRTX (iter, array, PATTERN (insn), ALL) > > - if (MEM_P (*iter) > > - && (reg_mentioned_p (stack_pointer_rtx, > > - *iter) > > - || reg_mentioned_p (frame_pointer_rtx, > > - *iter))) > > - { > > - unsigned int alignment = MEM_ALIGN (*iter); > > - if (alignment > stack_alignment) > > - stack_alignment = alignment; > > - } > > - } > > + break; > > } > > } > > > > cfun->machine->stack_frame_required = require_stack_frame; > > + > > + /* Stop if we don't need to check stack slot. */ > > + if (!check_stack_slot) > > + return; > > + > > + /* The preferred stack alignment is the minimum stack alignment. */ > > + if (stack_alignment > crtl->preferred_stack_boundary) > > + stack_alignment = crtl->preferred_stack_boundary; > > + > > + HARD_REG_SET stack_slot_access; > > + CLEAR_HARD_REG_SET (stack_slot_access); > > + > > + /* Stack slot can be accessed by stack pointer, frame pointer or > > + registers defined by stack pointer or frame pointer. */ > > + add_to_hard_reg_set (&stack_slot_access, Pmode, > > + STACK_POINTER_REGNUM); > > I'm possibly missing the point, but wasn't the purpose of the change to detect > "indirect" uses of the stack pointer? This now handles two levels, thus > *sp and tem = sp + 4; *tem, but it doesn't seem to handle tem2 = tem + 4; > *tem2? The previous patch did I think, but its compile-time complexity wasn't > sound. > > I think the ix86_find_all_reg_use wants to wrap itself inside sth like > > auto_bitmap worklist; > > bitmap_set_bit (worklist, reg); > do > { > int reg = bitmap_clear_first_set_bit (worklist); > /* it's current processing of reg */ > when it adds to stack_slot_access also add to worklist > } > while (!bitmap_empty_p (worklist));
Will fix. > > + ix86_find_all_reg_use (stack_slot_access, STACK_POINTER_REGNUM); > > + if (frame_pointer_needed) > > + { > > + add_to_hard_reg_set (&stack_slot_access, Pmode, > > + HARD_FRAME_POINTER_REGNUM); > > + ix86_find_all_reg_use (stack_slot_access, > > + HARD_FRAME_POINTER_REGNUM); > > + } > > + > > + for (int i = 0; i < FIRST_PSEUDO_REGISTER; i++) > > + if (GENERAL_REGNO_P (i) > > + && TEST_HARD_REG_BIT (stack_slot_access, i)) > > You can as well iterate over the stack_slot_access regset here? Will fix. Thanks. > > + for (df_ref ref = DF_REG_USE_CHAIN (i); > > + ref != NULL; > > + ref = DF_REF_NEXT_REG (ref)) > > + { > > + if (DF_REF_IS_ARTIFICIAL (ref)) > > + continue; > > + > > + rtx_insn *insn = DF_REF_INSN (ref); > > + if (!NONDEBUG_INSN_P (insn)) > > + continue; > > + note_stores (insn, ix86_update_stack_alignment, > > + &stack_alignment); > > + } > > } > > > > /* Finalize stack_realign_needed and frame_pointer_needed flags, which > > diff --git a/gcc/testsuite/g++.target/i386/pr109780-1.C > > b/gcc/testsuite/g++.target/i386/pr109780-1.C > > new file mode 100644 > > index 00000000000..7e3eabdec94 > > --- /dev/null > > +++ b/gcc/testsuite/g++.target/i386/pr109780-1.C > > @@ -0,0 +1,72 @@ > > +/* { dg-do compile } */ > > +/* { dg-require-effective-target c++17 } */ > > +/* { dg-options "-O2 -mavx2 -mtune=haswell" } */ > > + > > +template <typename _Tp> struct remove_reference { > > + using type = __remove_reference(_Tp); > > +}; > > +template <typename T> struct MaybeStorageBase { > > + T val; > > + struct Union { > > + ~Union(); > > + } mStorage; > > +}; > > +template <typename T> struct MaybeStorage : MaybeStorageBase<T> { > > + char mIsSome; > > +}; > > +template <typename T, typename U = typename remove_reference<T>::type> > > +constexpr MaybeStorage<U> Some(T &&); > > +template <typename T, typename U> constexpr MaybeStorage<U> Some(T > > &&aValue) { > > + return {aValue}; > > +} > > +template <class> struct Span { > > + int operator[](long idx) { > > + int *__trans_tmp_4; > > + if (__builtin_expect(idx, 0)) > > + *(int *)__null = false; > > + __trans_tmp_4 = storage_.data(); > > + return __trans_tmp_4[idx]; > > + } > > + struct { > > + int *data() { return data_; } > > + int *data_; > > + } storage_; > > +}; > > +struct Variant { > > + template <typename RefT> Variant(RefT) {} > > +}; > > +long from_i, from___trans_tmp_9; > > +namespace js::intl { > > +struct DecimalNumber { > > + Variant string_; > > + unsigned long significandStart_; > > + unsigned long significandEnd_; > > + bool zero_ = false; > > + bool negative_; > > + template <typename CharT> DecimalNumber(CharT string) : string_(string) > > {} > > + template <typename CharT> > > + static MaybeStorage<DecimalNumber> from(Span<const CharT>); > > + void from(); > > +}; > > +} // namespace js::intl > > +void js::intl::DecimalNumber::from() { > > + Span<const char16_t> __trans_tmp_3; > > + from(__trans_tmp_3); > > +} > > +template <typename CharT> > > +MaybeStorage<js::intl::DecimalNumber> > > +js::intl::DecimalNumber::from(Span<const CharT> chars) { > > + DecimalNumber number(chars); > > + if (auto ch = chars[from_i]) { > > + from_i++; > > + number.negative_ = ch == '-'; > > + } > > + while (from___trans_tmp_9 && chars[from_i]) > > + ; > > + if (chars[from_i]) > > + while (chars[from_i - 1]) > > + number.zero_ = true; > > + return Some(number); > > +} > > + > > +/* { dg-final { scan-assembler-not "and\[lq\]?\[^\\n\]*-32,\[^\\n\]*sp" } > > } */ > > diff --git a/gcc/testsuite/gcc.target/i386/pr109780-1.c > > b/gcc/testsuite/gcc.target/i386/pr109780-1.c > > new file mode 100644 > > index 00000000000..6b06947f2a5 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr109780-1.c > > @@ -0,0 +1,14 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O3 -march=skylake" } */ > > + > > +char perm[64]; > > + > > +void > > +__attribute__((noipa)) > > +foo (int n) > > +{ > > + for (int i = 0; i < n; ++i) > > + perm[i] = i; > > +} > > + > > +/* { dg-final { scan-assembler-not "and\[lq\]?\[^\\n\]*-32,\[^\\n\]*sp" } > > } */ > > diff --git a/gcc/testsuite/gcc.target/i386/pr109780-2.c > > b/gcc/testsuite/gcc.target/i386/pr109780-2.c > > new file mode 100644 > > index 00000000000..152da06c6ad > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr109780-2.c > > @@ -0,0 +1,21 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O3 -march=skylake" } */ > > + > > +#define N 9 > > + > > +void > > +f (double x, double y, double *res) > > +{ > > + y = -y; > > + for (int i = 0; i < N; ++i) > > + { > > + double tmp = y; > > + y = x; > > + x = tmp; > > + res[i] = i; > > + } > > + res[N] = y * y; > > + res[N + 1] = x; > > +} > > + > > +/* { dg-final { scan-assembler-not "and\[lq\]?\[^\\n\]*-32,\[^\\n\]*sp" } > > } */ > > -- > > 2.41.0 > > -- H.J.