This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] convert MIN_EXPR operands to the same type (PR 87059)


On Mon, Aug 27, 2018 at 5:27 PM Jeff Law <law@redhat.com> wrote:
>
> On 08/27/2018 02:32 AM, Richard Biener wrote:
> > On Sat, Aug 25, 2018 at 9:14 PM Jeff Law <law@redhat.com> wrote:
> >>
> >> On 08/24/2018 01:06 PM, Martin Sebor wrote:
> >>> PR 87059 points out an ICE in the recently enhanced VRP code
> >>> that was traced back to a MIN_EXPR built out of operands of
> >>> types with different sign by expand_builtin_strncmp().
> >>>
> >>> The attached patch adjusts the function to make sure both
> >>> operands have the same type, and to make these mismatches
> >>> easier to detect, also adds an assertion to fold_binary_loc()
> >>> for these expressions.
> >>>
> >>> Bootstrapped on x86_64-linux.
> >>>
> >>> Martin
> >>>
> >>> PS Aldy, I have not tested this on powerpc64le.
> >>>
> >>> gcc-87059.diff
> >>>
> >>>
> >>> PR tree-optimization/87059 - internal compiler error: in set_value_range
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>>       PR tree-optimization/87059
> >>>       * builtins.c (expand_builtin_strncmp): Convert MIN_EXPR operand
> >>>       to the same type as the other.
> >>>       * fold-const.c (fold_binary_loc): Assert expectation.
> >> I bootstrapped (but did not regression test) this on ppc64le and also
> >> built the linux kernel (which is where my tester tripped over this problem).
> >>
> >> Approved and installed on the trunk.
> >
> > Please remove the assertion in fold_binary_loc again, we do not do this kind
> > of assertions there.
> I almost called out the assertion.  We generally verify this kind of
> stuff in the verify_gimple routines and haphazard asserts in the folder
> would be just that -- haphazard.
>
> The value in Martin's assertion is to catch the goof earlier.  But I
> won't lose sleep if we drop the assert.

There's gazillion places where we could catch things "earlier" but I think
we have to be careful where we start placing stuff and if we do it do it
consistently.  MIN/MAX doesn't have much coverage so you won't see
latent issues (well, one just popped up in the testsuite for MIN/MAX...).

Note that fold_* really wants matching TYPE_MAIN_VARIANT but we
absolutely know we have entries from GIMPLE where that requirement
is not met!  So fold_* isn't a good place to do this kind of checks.

Richard.

> jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]