From: Richard Guenther <richard.guenther@gmail.com>
To: Kai Tietz <ktietz70@googlemail.com>
Cc: Kai Tietz <ktietz@redhat.com>, gcc-patches@gcc.gnu.org
Subject: Re: [patch gimplify]: Make sure comparison using boolean-type after gimplification
Date: Thu, 26 May 2011 12:18:00 -0000 [thread overview]
Message-ID: <BANLkTi=Z1ukXvp_iKhAgT6bLKM39EomzhA@mail.gmail.com> (raw)
In-Reply-To: <BANLkTi=OvFW104GYH1=1OezNX8iPYGoKsg@mail.gmail.com>
On Thu, May 26, 2011 at 1:28 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>:
>> On Thu, May 26, 2011 at 1:20 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>:
>>>> On Thu, May 26, 2011 at 1:11 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>>> 2011/5/26 Richard Guenther <richard.guenther@gmail.com>:
>>>>>> On Thu, May 26, 2011 at 12:46 PM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special.
>>>>>>>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types.
>>>>>>>>
>>>>>>>> ChangeLog
>>>>>>>>
>>>>>>>> 2011-05-26 Kai Tietz
>>>>>>>>
>>>>>>>> * gimplify.c (gimple_boolify): Boolify all comparison
>>>>>>>> expressions.
>>>>>>>> (gimplify_expr): Use 'useless_type_conversion_p' for comparing
>>>>>>>> org_type with boolean_type_node for TRUTH-expressions and comparisons.
>>>>>>>> * fold-const.c (fold_unary_loc): Handle comparison conversions with
>>>>>>>> boolean-type special.
>>>>>>>> * tree-cfg.c (verify_gimple_comparison): Adjust check for boolean
>>>>>>>> or compatible types.
>>>>>>>> (verify_gimple_assign_unary): Likewise.
>>>>>>>> * tree-ssa-forwprop.c (forward_propagate_comparison): Handle
>>>>>>>> boolean case special.
>>>>>>>>
>>>>>>>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply?
>>>>>>>
>>>>>>> It obviously isn't ok to apply before a patch has gone in that will resolve
>>>>>>> all of the FAILs you specify. Comments on the patch:
>>>>>>>
>>>>>>> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq
>>>>>>> plain wrong if bitfields are involved. */
>>>>>>> {
>>>>>>> tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1));
>>>>>>> + tree org_type = TREE_TYPE (*expr_p);
>>>>>>> +
>>>>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node))
>>>>>>> + {
>>>>>>> + TREE_TYPE (*expr_p) = boolean_type_node;
>>>>>>> + *expr_p = fold_convert_loc (saved_location, org_type, *expr_p);
>>>>>>> + ret = GS_OK;
>>>>>>> + goto dont_recalculate;
>>>>>>> + }
>>>>>>>
>>>>>>> The above should be only done for !AGGREGATE_TYPE_P. Probably then
>>>>>>> the strange dont_recalcuate goto can go away as well.
>>>>>>>
>>>>>>> if (!AGGREGATE_TYPE_P (type))
>>>>>>> - goto expr_2;
>>>>>>> + {
>>>>>>> + enum gimplify_status r0, r1;
>>>>>>> +
>>>>>>> + r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
>>>>>>> + post_p, is_gimple_val, fb_rvalue);
>>>>>>> + r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
>>>>>>> + post_p, is_gimple_val, fb_rvalue);
>>>>>>> +
>>>>>>> + ret = MIN (r0, r1);
>>>>>>> + }
>>>>>>> +
>>>>>>>
>>>>>>> why change this?
>>>>>>>
>>>>>>> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre
>>>>>>> }
>>>>>>> else if (COMPARISON_CLASS_P (arg0))
>>>>>>> {
>>>>>>> + /* Don't optimize type change, if op0 is of kind boolean_type_node.
>>>>>>> + Otherwise this will lead to race-condition on gimplification
>>>>>>> + trying to boolify comparison expression. */
>>>>>>> + if (TREE_TYPE (op0) == boolean_type_node)
>>>>>>> + return NULL_TREE;
>>>>>>> +
>>>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE)
>>>>>>> {
>>>>>>> arg0 = copy_node (arg0);
>>>>>>>
>>>>>>> The code leading here looks quite strange to me ...
>>>>>>>
>>>>>>> tree
>>>>>>> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0)
>>>>>>> {
>>>>>>> ...
>>>>>>> if (TREE_CODE_CLASS (code) == tcc_unary)
>>>>>>> {
>>>>>>> ...
>>>>>>> else if (COMPARISON_CLASS_P (arg0))
>>>>>>> {
>>>>>>> if (TREE_CODE (type) == BOOLEAN_TYPE)
>>>>>>> {
>>>>>>> arg0 = copy_node (arg0);
>>>>>>> TREE_TYPE (arg0) = type;
>>>>>>> return arg0;
>>>>>>> }
>>>>>>> else if (TREE_CODE (type) != INTEGER_TYPE)
>>>>>>> return fold_build3_loc (loc, COND_EXPR, type, arg0,
>>>>>>> fold_build1_loc (loc, code, type,
>>>>>>> integer_one_node),
>>>>>>> fold_build1_loc (loc, code, type,
>>>>>>> integer_zero_node));
>>>>>>> }
>>>>>>>
>>>>>>> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE,
>>>>>>> return arg0 ... sure. Same for the 2nd case. ~ (a == b) isn't
>>>>>>> the same as a == b ? ~1 : ~0. I _suppose_ those cases were
>>>>>>> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary,
>>>>>>> in which case they should be dropped or moved below where we
>>>>>>> handle conversions explicitly.
>>>>>>>
>>>>>>> That said - does anyone remember anything about that above code?
>>>>>>> Trying to do some svn blame history tracking now ...
>>>>>>
>>>>>> Oh, the patch continues...
>>>>>>
>>>>>> @@ -3208,7 +3208,10 @@ verify_gimple_comparison (tree type, tre
>>>>>> && (!POINTER_TYPE_P (op0_type)
>>>>>> || !POINTER_TYPE_P (op1_type)
>>>>>> || TYPE_MODE (op0_type) != TYPE_MODE (op1_type)))
>>>>>> - || !INTEGRAL_TYPE_P (type))
>>>>>> + || !(TREE_CODE (type) == BOOLEAN_TYPE
>>>>>> + || (TREE_TYPE (type) && TREE_CODE (type) == INTEGER_TYPE
>>>>>> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE)
>>>>>> + || (INTEGRAL_TYPE_P (type) && TYPE_PRECISION (type) == 1)))
>>>>>> {
>>>>>>
>>>>>> why that strange TREE_TYPE (TREE_TYPE ())) thing again? Drop
>>>>>> that.
>>>>>>
>>>>>> @@ -3352,6 +3355,8 @@ verify_gimple_assign_unary (gimple stmt)
>>>>>> case TRUTH_NOT_EXPR:
>>>>>> /* We require two-valued operand types. */
>>>>>> if (!(TREE_CODE (rhs1_type) == BOOLEAN_TYPE
>>>>>> + || (TREE_TYPE (rhs1_type) && TREE_CODE (rhs1_type) == INTEGER_TYPE
>>>>>> + && TREE_CODE (TREE_TYPE (rhs1_type)) == BOOLEAN_TYPE)
>>>>>> || (INTEGRAL_TYPE_P (rhs1_type)
>>>>>> && TYPE_PRECISION (rhs1_type) == 1)))
>>>>>> {
>>>>>>
>>>>>> likewise.
>>>>>
>>>>> Well, those checks are necessary for Ada and its crippled
>>>>> boolean_type_node and computed boolean-based integer construct. Ada
>>>>> derives here the boolean-type to an integer with range 0..1 and the
>>>>> only way to find out that it is in fact such a beast is by looking
>>>>> into TREE_TYPE of type. See here Ada's code for getting base-type
>>>>> information.
>>>>> As such things are treated as compatible they can appear for TRUTH_NOT
>>>>> expressions and comparisons.
>>>>
>>>> No they can't as we boolify them.
>>>>
>>>> Richard.
>>>
>>> Well, they do. Test can prove this by running fortran's dg tests.
>>>
>>> The logic we do in gimplification is:
>>>
>>> 1) save original type of expression.
>>> 2) set expression's type to boolean_type_node.
>>> 3) if conversion from boolean_type_node to saved type isn't useless
>>> (and here we have the issue about Fortran's different booleans with
>>> different modes, which make those kinds not useless) then cast
>>> expression's result to original type and retry gimplification.
>>> 4) Otherwise gimplify operands and continue.
>>
>> You don't make sense. Btw,
>>
>>>>>> + if (!useless_type_conversion_p (org_type, boolean_type_node))
>>>>>> + {
>>>>>> + TREE_TYPE (*expr_p) = boolean_type_node;
>>>>>> + *expr_p = fold_convert_loc (saved_location, org_type,
>>
>> is bogus it should be
>>
>> TREE_TYPE (*expr_p) = boolean_type_node;
>> if (!useless_type_conversion_p (org_type, boolean_type_node))
>> {
>> *expr_p = fold_convert_loc (saved_location, org_type,
>
> Well, I spared here the type conversion to boolean_type_node for
> useless conversion,
useless_type_conversion_p isn't symmetric, so you can't do that.
> but it doesn't change anything about the endless
> recursion here. As long as fold_convert_loc modifies *expr_p's type
> back to original type it will loop for ever for those cases I
> described above.
Indeed which is why we also touch fold-const.c!
> Please don't forget that boolean_type_node is FE type here and not the
> later 1-bit BOOL_SIZE type set in free_lang_data.
I perfectly know that. Still no valid reason.
Richard.
> Kai
>
next prev parent reply other threads:[~2011-05-26 11:30 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <373566883.242226.1306404169706.JavaMail.root@zmail06.collab.prod.int.phx2.redhat.com>
2011-05-26 10:56 ` Kai Tietz
2011-05-26 11:12 ` Richard Guenther
2011-05-26 11:20 ` Richard Guenther
2011-05-26 11:52 ` Kai Tietz
2011-05-26 12:04 ` Richard Guenther
2011-05-26 12:05 ` Kai Tietz
2011-05-26 12:05 ` Richard Guenther
2011-05-26 12:06 ` Kai Tietz
2011-05-26 12:18 ` Richard Guenther [this message]
2011-05-26 12:23 ` Kai Tietz
2011-05-26 11:30 ` Richard Guenther
2011-06-06 16:10 ` Diego Novillo
2011-05-26 11:39 ` Kai Tietz
2011-05-26 12:00 ` Richard Guenther
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='BANLkTi=Z1ukXvp_iKhAgT6bLKM39EomzhA@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=ktietz70@googlemail.com \
--cc=ktietz@redhat.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).