public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
From: "cvs-commit at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug tree-optimization/99079] [8/9 Regression] Maybe a wrong code since r6-1462-g4ab1e111ef0669bb
Date: Thu, 22 Apr 2021 16:50:51 +0000	[thread overview]
Message-ID: <bug-99079-4-gdBu8cSF1L@http.gcc.gnu.org/bugzilla/> (raw)
In-Reply-To: <bug-99079-4@http.gcc.gnu.org/bugzilla/>

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99079

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-8 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:83b2007abb2ad05209483e8b9f75aec9b385bbb9

commit r8-10885-g83b2007abb2ad05209483e8b9f75aec9b385bbb9
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Feb 15 09:16:06 2021 +0100

    match.pd: Fix up A % (cast) (pow2cst << B) simplification [PR99079]

    The (mod @0 (convert?@3 (power_of_two_cand@1 @2))) simplification
    uses tree_nop_conversion_p (type, TREE_TYPE (@3)) condition, but I believe
    it doesn't check what it was meant to check.  On convert?@3
    TREE_TYPE (@3) is not the type of what it has been converted from, but
    what it has been converted to, which needs to be (because it is operand
    of normal binary operation) equal or compatible to type of the modulo
    result and first operand - type.
    I could fix that by using && tree_nop_conversion_p (type, TREE_TYPE (@1))
    and be done with it, but actually most of the non-nop conversions are IMHO
    ok and so we would regress those optimizations.
    In particular, if we have say narrowing conversions (foo5 and foo6 in
    the new testcase), I think we are fine, either the shift of the power of
two
    constant after narrowing conversion is still that power of two (or negation
    of that) and then it will still work, or the result of narrowing conversion
    is 0 and then we would have UB which we can ignore.
    Similarly, widening conversions where the shift result is unsigned are
fine,
    or even widening conversions where the shift result is signed, but we sign
    extend to a signed wider divisor, the problematic case of INT_MIN will
    become x % (long long) INT_MIN and we can still optimize that to
    x & (long long) INT_MAX.
    What doesn't work is the case in the pr99079.c testcase, widening
conversion
    of a signed shift result to wider unsigned divisor, where if the shift
    is negative, we end up with x % (unsigned long long) INT_MIN which is
    x % 0xffffffff80000000ULL where the divisor is not a power of two and
    we can't optimize that to x & 0x7fffffffULL.

    So, the patch rejects only the single problematic case.

    Furthermore, when the shift result is signed, we were introducing UB into
    a program which previously didn't have one (well, left shift into the sign
    bit is UB in some language/version pairs, but it is definitely valid in
    C++20 - wonder if I shouldn't move the gcc.c-torture/execute/pr99079.c
    testcase to g++.dg/torture/pr99079.C and use -std=c++20), by adding that
    subtraction of 1, x % (1 << 31) in C++20 is well defined, but
    x & ((1 << 31) - 1) triggers UB on the subtraction.
    So, the patch performs the subtraction in the unsigned type if it isn't
    wrapping.

    2021-02-15  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/99079
            * match.pd (A % (pow2pcst << N) -> A & ((pow2pcst << N) - 1)):
Remove
            useless tree_nop_conversion_p (type, TREE_TYPE (@3)) check. 
Instead
            require both type and TREE_TYPE (@1) to be integral types and
either
            type having smaller or equal precision, or TREE_TYPE (@1) being
            unsigned type, or type being signed type.  If TREE_TYPE (@1)
            doesn't have wrapping overflow, perform the subtraction of one in
            unsigned type.

            * gcc.dg/fold-modpow2-2.c: New test.
            * gcc.c-torture/execute/pr99079.c: New test.

    (cherry picked from commit 45de8afb2d534e3b38b4d1898686b20c29cc6a94)

  parent reply	other threads:[~2021-04-22 16:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-12 17:22 [Bug tree-optimization/99079] New: " marxin at gcc dot gnu.org
2021-02-12 17:22 ` [Bug tree-optimization/99079] " marxin at gcc dot gnu.org
2021-02-12 17:51 ` jakub at gcc dot gnu.org
2021-02-12 18:56 ` jakub at gcc dot gnu.org
2021-02-12 19:09 ` jakub at gcc dot gnu.org
2021-02-12 19:11 ` jakub at gcc dot gnu.org
2021-02-12 19:21 ` jakub at gcc dot gnu.org
2021-02-15  8:17 ` cvs-commit at gcc dot gnu.org
2021-02-15  8:20 ` [Bug tree-optimization/99079] [8/9/10 Regression] " jakub at gcc dot gnu.org
2021-03-19 23:29 ` cvs-commit at gcc dot gnu.org
2021-03-20  8:07 ` [Bug tree-optimization/99079] [8/9 " jakub at gcc dot gnu.org
2021-04-20 23:32 ` cvs-commit at gcc dot gnu.org
2021-04-22 16:50 ` cvs-commit at gcc dot gnu.org [this message]
2021-04-22 17:11 ` jakub at gcc dot gnu.org

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bug-99079-4-gdBu8cSF1L@http.gcc.gnu.org/bugzilla/ \
    --to=gcc-bugzilla@gcc.gnu.org \
    --cc=gcc-bugs@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).