From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27985 invoked by alias); 30 Mar 2017 12:44:30 -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 27967 invoked by uid 89); 30 Mar 2017 12:44:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-oi0-f45.google.com Received: from mail-oi0-f45.google.com (HELO mail-oi0-f45.google.com) (209.85.218.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 30 Mar 2017 12:44:26 +0000 Received: by mail-oi0-f45.google.com with SMTP id o67so29857810oib.1 for ; Thu, 30 Mar 2017 05:44:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=ry5RiHgoc4wf7j+GXsT1otGcBrHsri7RO9KMlB43zbs=; b=WQn0ns4QN+KoEHedX4RsgcrEyF6WIzt7JhdRs57sYZDGuPBgwAfPt0OUKBmC/sDf2H e5FYIDtf0aKwrBOped0hgAVsJrg43H+UB/17T6LNG5W49pdIMEnIfyvF0yBusbcf5ga2 CH+0oz6+HhMER6iqx4Albj7j6cguupe4IF+oefKKibb93WvHrnabE3neMYi5pxn0QYyl GvGEunh/hrl2BQi8TmN6vJiDnQFKwZB3JZcDTCG1T8lAMJ1d20LDJPnukVPZo1QyyR5V 1dRWaBt7IqpkDeEDC/0wis14ISU29R/srbWu66Z9xvouTqmlytYD4zfr5BJjoxf/juRR mYGw== X-Gm-Message-State: AFeK/H02999awa8REzr818bYA5xBYKVe5b6Yw3DZZdTOzLm6WQPAVGUwI0XF3CERYS4e6xotL8bpqEruBO6M6A== X-Received: by 10.202.5.1 with SMTP id 1mr2954638oif.0.1490877865870; Thu, 30 Mar 2017 05:44:25 -0700 (PDT) MIME-Version: 1.0 Received: by 10.157.83.4 with HTTP; Thu, 30 Mar 2017 05:44:25 -0700 (PDT) In-Reply-To: References: From: Richard Biener Date: Thu, 30 Mar 2017 13:00:00 -0000 Message-ID: Subject: Re: [PATCH PR80153]Always generate folded type conversion in tree-affine To: "Bin.Cheng" Cc: "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg01519.txt.bz2 On Thu, Mar 30, 2017 at 2:03 PM, Bin.Cheng wrote: > On Thu, Mar 30, 2017 at 11:37 AM, Richard Biener > wrote: >> On Wed, Mar 29, 2017 at 5:22 PM, Bin.Cheng wrote: >>> On Tue, Mar 28, 2017 at 1:34 PM, Richard Biener >>> wrote: >>>> On Tue, Mar 28, 2017 at 2:01 PM, Bin Cheng wrote: >>>>> Hi, >>>>> This patch is to fix PR80153. As analyzed in the PR, root cause is tree_affine lacks >>>>> ability differentiating (unsigned)(ptr + offset) and (unsigned)ptr + (unsigned)offset, >>>>> even worse, it always returns the former expression in aff_combination_tree, which >>>>> is wrong if the original expression has the latter form. The patch resolves the issue >>>>> by always returning the latter form expression, i.e, always trying to generate folded >>>>> expression. Also as analyzed in comment, I think this change won't result in substantial >>>>> code gen difference. >>>>> I also need to adjust get_computation_aff for test case gcc.dg/tree-ssa/reassoc-19.c. >>>>> Well, I think the changed behavior is correct, but for case the original pointer candidate >>>>> is chosen, it should be unnecessary to compute in uutype. Also this adjustment only >>>>> generates (unsigned)(pointer + offset) which is generated by tree-affine.c. >>>>> Bootstrap and test on x86_64 and AArch64. Is it OK? >>>> >>> Thanks for reviewing. >>>> Hmm. What is the desired goal? To have all elts added have >>>> comb->type as type? Then >>>> the type passed to add_elt_to_tree is redundant with comb->type. It >>>> looks like it >>>> is always passed comb->type now. >>> Yes, except pointer type comb->type, elts are converted to comb->type >>> with this patch. >>> The redundant type is removed in updated patch. >>> >>>> >>>> ISTR from past work in this area that it was important for pointer >>>> combinations to allow >>>> both pointer and sizetype elts at least. >>> Yes, It's still important to allow different types for pointer and >>> offset in pointer type comb. >>> I missed a pointer type check condition in the patch, fixed in updated patch. >>>> >>>> Your change is incomplete I think, for the scale == -1 and POINTER_TYPE_P case >>>> elt is sizetype now, not of pointer type. As said above, we are >>>> trying to maintain >>>> both pointer and sizetype elts with like: >>>> >>>> if (scale == 1) >>>> { >>>> if (!expr) >>>> { >>>> if (POINTER_TYPE_P (TREE_TYPE (elt))) >>>> return elt; >>>> else >>>> return fold_convert (type1, elt); >>>> } >>>> >>>> where your earilier fold to type would result in not all cases handled the same >>>> (depending whether scale was -1 for example). >>> IIUC, it doesn't matter. For comb->type being pointer type, the >>> behavior remains the same. >>> For comb->type being unsigned T, this elt is converted to ptr_offtype, >>> rather than unsigned T, >>> this doesn't matter because ptr_offtype and unsigned T are equal to >>> each other, otherwise >>> tree_to_aff_combination shouldn't distribute it as a single elt. >>> Anyway, this is addressed in updated patch by checking pointer >>> comb->type additionally. >>> BTW, I think "scale==-1" case is a simple heuristic differentiating >>> pointer_base and offset. >>> >>>> >>>> Thus - shouldn't we simply drop the type argument (or rather the comb one? >>>> that wide_int_ext_for_comb looks weird given we get a widest_int as input >>>> and all the other wide_int_ext_for_comb calls around). >>>> >>>> And unconditionally convert to type, simplifying the rest of the code? >>> As said, for pointer type comb, we need to keep current behavior; for >>> other cases, >>> unconditionally convert to comb->type is the goal. >>> >>> Bootstrap and test on x86_64 and AArch64. Is this version OK? >> >> @@ -399,22 +400,20 @@ add_elt_to_tree (tree expr, tree type, tree elt, >> const widest_int &scale_in, >> if (POINTER_TYPE_P (TREE_TYPE (elt))) >> return elt; >> else >> - return fold_convert (type1, elt); >> + return fold_convert (type, elt); >> } >> >> the conversion should already have been done. For non-pointer comb->type >> it has been converted to type by your patch. For pointer-type comb->type >> it should be either pointer type or ptrofftype ('type') already as well. >> >> That said, can we do sth like >> >> @@ -384,6 +395,12 @@ add_elt_to_tree (tree expr, tree type, t >> >> widest_int scale = wide_int_ext_for_comb (scale_in, comb); >> >> + if (! POINTER_TYPE_P (comb->type)) >> + elt = fold_convert (comb->type, elt); >> + else >> + gcc_assert (POINTER_TYPE_P (TREE_TYPE (elt)) >> + || types_compatible_p (TREE_TYPE (elt), type1)); > Hmm, this assert can be broken since we do STRIP_NOPS converting to > aff_tree. It's not compatible for signed and unsigned integer types. > Also, with this patch, we can even support elt of short type in a > unsigned long comb, though this is useless. > >> + >> if (scale == -1 >> && POINTER_TYPE_P (TREE_TYPE (elt))) >> { >> >> that is clearly do the conversion at the start in a way the state >> of elt is more clear? > Yes, thanks. V3 patch attached (with gcc_assert removed). Is it ok > after bootstrap/test? - return fold_build2 (PLUS_EXPR, type1, - expr, fold_convert (type1, elt)); + return fold_build2 (PLUS_EXPR, type, expr, fold_convert (type, elt)); folding not needed(?) - return fold_build1 (NEGATE_EXPR, type1, - fold_convert (type1, elt)); + return fold_build1 (NEGATE_EXPR, type, fold_convert (type, elt)); likewise. - return fold_build2 (MINUS_EXPR, type1, - expr, fold_convert (type1, elt)); + return fold_build2 (MINUS_EXPR, type, expr, fold_convert (type, elt)); likewise. Ok with removing those and re-testing. Thanks, Richard. > Thanks, > bin >> >> Richard. >> >> >> >>> Thanks, >>> bin >>> >>> 2017-03-28 Bin Cheng >>> >>> PR tree-optimization/80153 >>> * tree-affine.c (add_elt_to_tree): Remove parameter TYPE. Use type >>> of parameter COMB. Convert elt to type of COMB it COMB is not of >>> pointer type. >>> (aff_combination_to_tree): Update calls to add_elt_to_tree. >>> * tree-ssa-loop-ivopts.c (alloc_iv): Pass in consistent types. >>> (get_computation_aff): Use utype directly for original candidate. >>> >>> gcc/testsuite/ChangeLog >>> 2017-03-28 Bin Cheng >>> >>> PR tree-optimization/80153 >>> * gcc.c-torture/execute/pr80153.c: New.