From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117723 invoked by alias); 10 Oct 2016 08:47:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 117706 invoked by uid 89); 10 Oct 2016 08:47:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.0 required=5.0 tests=AWL,BAYES_05,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=unpatched, appeal, H*RU:sk:mail2-r, H*r:192.134.164 X-HELO: mail2-relais-roc.national.inria.fr Received: from mail2-relais-roc.national.inria.fr (HELO mail2-relais-roc.national.inria.fr) (192.134.164.83) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 10 Oct 2016 08:47:27 +0000 Received: from ip-118.net-89-2-234.rev.numericable.fr (HELO laptop-mg.local) ([89.2.234.118]) by mail2-relais-roc.national.inria.fr with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Oct 2016 10:46:58 +0200 Date: Mon, 10 Oct 2016 08:47:00 -0000 From: Marc Glisse Reply-To: gcc-patches@gcc.gnu.org To: Richard Biener cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix PR77826 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset=US-ASCII X-SW-Source: 2016-10/txt/msg00575.txt.bz2 On Fri, 7 Oct 2016, Richard Biener wrote: > On Thu, 6 Oct 2016, Marc Glisse wrote: > >> On Wed, 5 Oct 2016, Richard Biener wrote: >> >>>> The following will fix PR77826, the issue that in match.pd matching >>>> up two things uses operand_equal_p which is too lax about the type >>>> of the toplevel entity (at least for integer constants). >>>> >>>> Bootstrap / regtest pending on x86_64-unknown-linux-gnu. >>> >>> The following is what I have applied. >>> >>> Richard. >>> >>> 2016-10-05 Richard Biener >>> >>> PR middle-end/77826 >>> * genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p >>> with types_match for GIMPLE code gen to handle type mismatched >>> constants properly. >> >> I don't understand the disparity between generic and gimple here. Why let >> (char)1 and (long)1 match in generic but not in gimple? And there are probably >> several transformations in match.pd that could do with an update if constants >> don't match anymore. Or did I misunderstand what the patch does? > > The disparity is mostly that with GENERIC unfolded trees such as (char)1 > are a bug while in GIMPLE the fact that the match.pd machinery does > valueization makes those a "feature" we have to deal with. Originally > I've restricted GENERIC as well but with its types_match_p implementation > it resulted in too many missed matches. I shouldn't have written (long)1, I meant the fact that 1 (as a char constant) and 1 (as a long constant) will now be matching captures in generic and not in gimple. If we are going in the direction of not matching constants of different types, I'd rather do it consistently and update the patterns as needed to avoid the missed optimizations. The missed matches exist in gimple as well, and generic optimization seems less important than gimple to me. An example that regressed at -O (looking at the .optimized dump) int f(int a, unsigned b){ a &= 1; b &= 1; return a&b; } If we stick to the old behavior, maybe we could have some genmatch magic to help with the constant capture weirdness. With matching captures, we could select which operand (among those supposed to be equivalent) is actually captured more cleverly, either with an explicit marker, or by giving priority to the one that is not immediatly below convert? in the pattern. And if we move to stricter matching, maybe genmatch could be updated so convert could also match integer constants in some cases. > I agree that some transforms would need updates - I've actually tried > to implement a warning for genmatch whenever seeing a match with > (T)@0 but there isn't any good existing place to sneak that in. >>> * match.pd ((X /[ex] A) * A -> X): Properly handle converted >>> and constant A. >> >> This regressed >> int f(int*a,int*b){return 4*(int)(b-a);} > > This is because (int)(b-a) could be a truncation in which case > multiplying with 4 might not result in the same value as > b-a truncated(?). The comment before the unpatched patterns > said "sign-changing conversions" but nothign actually verified this. > Might be that truncations are indeed ok now that I think about it. 2015-05-22 Marc Glisse PR tree-optimization/63387 * match.pd ((X /[ex] A) * A -> X): Remove unnecessary condition. Apparently I forgot to remove the comment at that time :-( > Btw, do you have a better suggestion as to how to handle the original > issue rather than not relying on operand_equal_p for constants? In previous cases, in order to get the right version of a matching capture, we used non-matching captures and an explicit call to operand_equal_p, for instance: /* X - (X / Y) * Y is the same as X % Y. */ (simplify (minus (convert1? @2) (convert2? (mult:c (trunc_div @0 @1) @1))) /* We cannot use matching captures here, since in the case of constants we really want the type of @0, not @2. */ (if (operand_equal_p (@0, @2, 0) && (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))) (convert (trunc_mod @0 @1)))) That seems to match the solutions that Jakub and you were discussing in the PR, something like it should work (we can discuss the exact code). I don't know if it is better. The old behavior of matching captures with inconsistent types was confusing. A behavior with strict matching may complicate things (will we duplicate patterns, or write operand_equal_p explicitly to mimic the old behavior?). The recent inconsistency between generic and gimple doesnt appeal to me much... -- Marc Glisse