On Mon, Aug 27, 2018 at 5:27 PM Jeff Law <l...@redhat.com> wrote: > > On 08/27/2018 02:32 AM, Richard Biener wrote: > > On Sat, Aug 25, 2018 at 9:14 PM Jeff Law <l...@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
