From: Andrew Pinski <pinskia@gmail.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq
Date: Fri, 21 Jul 2017 17:16:00 -0000 [thread overview]
Message-ID: <CA+=Sn1=eov2Kaky94cTANEFDfBpuJEf5_2eftRw4FEz=njdYMQ@mail.gmail.com> (raw)
In-Reply-To: <6B04BC1D-13FA-4AED-90D1-6AD6BF10BDD2@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4798 bytes --]
On Wed, Jul 19, 2017 at 11:13 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On July 19, 2017 6:10:28 PM GMT+02:00, Andrew Pinski <pinskia@gmail.com> wrote:
>>On Mon, Jul 17, 2017 at 3:02 AM, Richard Biener
>><richard.guenther@gmail.com> wrote:
>>> On Thu, Jul 13, 2017 at 6:18 AM, Andrew Pinski <pinskia@gmail.com>
>>wrote:
>>>> On Wed, Jul 12, 2017 at 9:10 PM, Marc Glisse <marc.glisse@inria.fr>
>>wrote:
>>>>> On Wed, 12 Jul 2017, Andrew Pinski wrote:
>>>>>
>>>>>> Hi,
>>>>>> Unlike most other expressions, BIT_INSERT_EXPR has an implicit
>>>>>> operand of the precision/size of the second operand. This means
>>if we
>>>>>> have an integer constant for the second operand and that compares
>>to
>>>>>> the same constant value, vn_nary_op_eq would return that these two
>>>>>> expressions are the same. But in the case I was looking into the
>>>>>> integer constants had different types, one with 1 bit precision
>>and
>>>>>> the other with 2 bit precision which means the BIT_INSERT_EXPR
>>were
>>>>>> not equal at all.
>>>>>>
>>>>>> This patches the problem by checking to see if BIT_INSERT_EXPR's
>>>>>> operand 1's (second operand) type has different precision to
>>return
>>>>>> false.
>>>>>>
>>>>>> Is this the correct location or should we be checking for this
>>>>>> differently? If this is the correct location, is the patch ok?
>>>>>> Bootstrapped and tested on aarch64-linux-gnu with no regressions
>>(and
>>>>>> also tested with a few extra patches to expose BIT_INSERT_EXPR).
>>>>>>
>>>>>> Thanks,
>>>>>> Andrew Pinski
>>>>>>
>>>>>> ChangeLog:
>>>>>> * tree-ssa-sccvn.c (vn_nary_op_eq): Check BIT_INSERT_EXPR's
>>operand 1
>>>>>> to see if the types precision matches.
>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> since BIT_INSERT_EXPR is implicitly an expression with 4 arguments,
>>it makes
>>>>> sense that we may need a few such special cases. But shouldn't the
>>hash
>>>>> function be in sync with the equality comparator? Does
>>operand_equal_p need
>>>>> the same?
>>>>
>>>> The hash function does not need to be exactly the same. The only
>>>> requirement there is if vn_nary_op_eq returns true then the hash has
>>>> to be the same. Now we could improve the hash by using the
>>precision
>>>> which will allow us not to compare as much in some cases.
>>>>
>>>> Yes operand_equal_p needs the same handling; I did not notice that
>>>> until you mention it..
>>>> Right now it does:
>>>> case BIT_INSERT_EXPR:
>>>> return OP_SAME (0) && OP_SAME (1) && OP_SAME (2);
>>>
>>> Aww. The issue is that operand_equal_p treats INTEGER_CSTs of
>>different
>>> type/precision but the same value as equal.
>>>
>>> Revisiting that, while a good idea, shouldn't block a fix here. So
>>...
>>>
>>> Index: tree-ssa-sccvn.c
>>> ===================================================================
>>> --- tree-ssa-sccvn.c (revision 250159)
>>> +++ tree-ssa-sccvn.c (working copy)
>>> @@ -2636,6 +2636,14 @@ vn_nary_op_eq (const_vn_nary_op_t const
>>> if (!expressions_equal_p (vno1->op[i], vno2->op[i]))
>>> return false;
>>>
>>> + /* BIT_INSERT_EXPR has an implict operand as the type precision
>>> + of op1. Need to check to make sure they are the same. */
>>> + if (vno1->opcode == BIT_INSERT_EXPR)
>>> + if (INTEGRAL_TYPE_P (TREE_TYPE (vno1->op[0]))
>>> + && TYPE_PRECISION (TREE_TYPE (vno1->op[1]))
>>> + != TYPE_PRECISION (TREE_TYPE (vno2->op[1])))
>>> + return false;
>>> +
>>>
>>> the case can be restricted to INTEGER_CST vno1->op[0] I think:
>>>
>>> if (vno1->opcode == BIT_INSERT_EXPR
>>> && TREE_CODE (vno1->op[0]) == INTEGER_CST
>>> && TYPE_PRECISION (....
>>>
>>> and yes, operand_equal_p needs a similar fix. Can you re-post with
>>that added?
>>
>>Here is that with the changes you requested too.
>>
>>> Do you have a testcase?
>>
>>I don't have one which fails with the trunk. With lowering of
>>bit-fields accesses (which I hope to submit soon; just getting in the
>>required patches first), many testcases fail (bootstrap fails for the
>>same reason too).
>>
>>OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>
> In the fold-const.c hunk you need to verify arg1 op0 is an INTEGER_CST. This is not necessary in sccvn because we passed operand_equal_p first.
>
> OK with that change.
This is what I applied in the end. It is not op0 that needs to be
changed here but rather op1; I had a typo in the fold-const.c check.
Thanks,
Andrew
>
> Richard.
>
>>Thanks,
>>Andrew
>>
>>ChangeLog:
>>* tree-ssa-sccvn.c (vn_nary_op_eq): Check BIT_INSERT_EXPR's operand 1
>>to see if the types precision matches.
>>* fold-const.c (operand_equal_p): Likewise,
>>
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks,
>>>> Andrew Pinski
>>>>
>>>>>
>>>>> --
>>>>> Marc Glisse
>
[-- Attachment #2: fixbitinsert-vn.diff.txt --]
[-- Type: text/plain, Size: 1532 bytes --]
Index: fold-const.c
===================================================================
--- fold-const.c (revision 250430)
+++ fold-const.c (working copy)
@@ -3184,9 +3184,18 @@ operand_equal_p (const_tree arg0, const_
flags &= ~OEP_ADDRESS_OF;
return OP_SAME (0);
+ case BIT_INSERT_EXPR:
+ /* BIT_INSERT_EXPR has an implict operand as the type precision
+ of op1. Need to check to make sure they are the same. */
+ if (TREE_CODE (TREE_OPERAND (arg0, 1)) == INTEGER_CST
+ && TREE_CODE (TREE_OPERAND (arg1, 1)) == INTEGER_CST
+ && TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (arg0, 1)))
+ != TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (arg1, 1))))
+ return false;
+ /* FALLTHRU */
+
case VEC_COND_EXPR:
case DOT_PROD_EXPR:
- case BIT_INSERT_EXPR:
return OP_SAME (0) && OP_SAME (1) && OP_SAME (2);
case MODIFY_EXPR:
Index: tree-ssa-sccvn.c
===================================================================
--- tree-ssa-sccvn.c (revision 250430)
+++ tree-ssa-sccvn.c (working copy)
@@ -2636,6 +2636,14 @@ vn_nary_op_eq (const_vn_nary_op_t const
if (!expressions_equal_p (vno1->op[i], vno2->op[i]))
return false;
+ /* BIT_INSERT_EXPR has an implict operand as the type precision
+ of op1. Need to check to make sure they are the same. */
+ if (vno1->opcode == BIT_INSERT_EXPR
+ && TREE_CODE (vno1->op[1]) == INTEGER_CST
+ && TYPE_PRECISION (TREE_TYPE (vno1->op[1]))
+ != TYPE_PRECISION (TREE_TYPE (vno2->op[1])))
+ return false;
+
return true;
}
prev parent reply other threads:[~2017-07-21 17:16 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-13 1:19 Andrew Pinski
2017-07-13 4:14 ` Marc Glisse
2017-07-13 4:18 ` Andrew Pinski
2017-07-17 10:03 ` Richard Biener
2017-07-19 16:10 ` Andrew Pinski
2017-07-19 18:13 ` Richard Biener
2017-07-21 17:16 ` Andrew Pinski [this message]
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='CA+=Sn1=eov2Kaky94cTANEFDfBpuJEf5_2eftRw4FEz=njdYMQ@mail.gmail.com' \
--to=pinskia@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=richard.guenther@gmail.com \
/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).