On Fri, Jun 21, 2024 at 10:51 PM Roger Sayle <ro...@nextmovesoftware.com> wrote:
>
>
> Hi Richard,
> Thanks for the review and apologies for taking so long to get back to this.
> This revision implements your suggestions from early May, as found at
> https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650405.html
>
> This patch fixes PR tree-optimization/113673, a P2 ice-on-valid regression
> caused by load merging of (ptr[0]<<8)+ptr[1] when -ftrapv has been
> specified. When the operator is | or ^ this is safe, but for addition
> of signed integer types, a trap may be generated/required, so merging this
> idiom into a single non-trapping instruction is inappropriate, confusing
> the compiler by transforming a basic block with an exception edge into one
> without.
>
> This revision implements Richard Biener's feedback to add an early check
> for stmt_can_throw_internal (cfun, stmt) to prevent transforming in the
> presence of any statement that could trap, not just overflow on addition.
> The one other tweak included in this patch is to mark the local function
> find_bswap_or_nop_load as static ensuring that it isn't called from outside
> this file, and guaranteeing that it is dominated by stmt_can_throw_internal
> checking.
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32}
> with no new failures. Ok for mainline?
OK.
Thanks,
Richard.
>
> 2024-06-21 Roger Sayle <ro...@nextmovesoftware.com>
> Richard Biener <rguent...@suse.de>
>
> gcc/ChangeLog
> PR tree-optimization/113673
> * gimple-ssa-store-merging.cc (find_bswap_or_nop_load): Make static.
> (find_bswap_or_nop_1): Avoid transformations (load merging) when
> stmt_can_throw_internal indicates that a statement can trap.
>
> gcc/testsuite/ChangeLog
> PR tree-optimization/113673
> * g++.dg/pr113673.C: New test case.
>
> Thanks in advance,
> Roger
> --
>
> > -----Original Message-----
> > From: Richard Biener <richard.guent...@gmail.com>
> > Sent: 02 May 2024 10:27
> > Subject: Re: [PATCH] PR tree-opt/113673: Avoid load merging from potentially
> > trapping additions.
> >
> > On Sun, Apr 28, 2024 at 11:11 AM Roger Sayle <ro...@nextmovesoftware.com>
> > wrote:
> > >
> > > This patch fixes PR tree-optimization/113673, a P2 ice-on-valid
> > > regression caused by load merging of (ptr[0]<<8)+ptr[1] when -ftrapv
> > > has been specified. When the operator is | or ^ this is safe, but for
> > > addition of signed integer types, a trap may be generated/required, so
> > > merging this idiom into a single non-trapping instruction is
> > > inappropriate, confusing the compiler by transforming a basic block
> > > with an exception edge into one without. One fix is to be more
> > > selective for PLUS_EXPR than for BIT_IOR_EXPR or BIT_XOR_EXPR in
> > > gimple-ssa-store-merging.cc's
> > > find_bswap_or_nop_1 function.
> > >
> > > An alternate solution might be to notice that in this idiom the
> > > addition can't overflow, but that this detail wasn't apparent when
> > > exception edges were added to the CFG. In which case, it's safe to
> > > remove (or mark for
> > > removal) the problematic exceptional edge. Unfortunately updating the
> > > CFG is a part of the compiler that I'm less familiar with.
> > >
> > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> > > and make -k check, both with and without --target_board=unix{-m32}
> > > with no new failures. Ok for mainline?
> >
> > Instead of
> >
> > + case PLUS_EXPR:
> > + /* Don't perform load merging if this addition can trap. */
> > + if (cfun->can_throw_non_call_exceptions
> > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs1))
> > + && TYPE_OVERFLOW_TRAPS (TREE_TYPE (rhs1)))
> > + return NULL;
> >
> > please check stmt_can_throw_internal (cfun, stmt) - the
> > find_bswap_or_no_load
> > call in the function suffers from the same issue, so this should probably be
> > checked before that call even.
> >
> > Thanks,
> > Richard.
> >
> > >
> > > 2024-04-28 Roger Sayle <ro...@nextmovesoftware.com>
> > >
> > > gcc/ChangeLog
> > > PR tree-optimization/113673
> > > * gimple-ssa-store-merging.cc (find_bswap_or_nop_1) <case
> > > PLUS_EXPR>:
> > > Don't perform load merging if a signed addition may trap.
> > >
> > > gcc/testsuite/ChangeLog
> > > PR tree-optimization/113673
> > > * g++.dg/pr113673.C: New test case.
> > >
>
