* [PATCH] Fix bug in simplify_ternary_operation
@ 2017-08-28 18:50 Tom de Vries
2017-08-30 9:12 ` Tom de Vries
2017-08-31 23:37 ` Jeff Law
0 siblings, 2 replies; 7+ messages in thread
From: Tom de Vries @ 2017-08-28 18:50 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]
Hi,
I think I found a bug in r17465:
...
> * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE
> simplifications.
>
> diff --git a/gcc/cse.c b/gcc/cse.c
> index e001597..3c27387 100644
> --- a/gcc/cse.c
> +++ b/gcc/cse.c
> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
Note: the parameters of simplify_ternary_operation have the following
meaning:
...
/* Simplify CODE, an operation with result mode MODE and three operands,
OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became
a constant. Return 0 if no simplifications is possible. */
rtx
simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
enum rtx_code code;
enum machine_mode mode, op0_mode;
rtx op0, op1, op2;
...
> && rtx_equal_p (XEXP (op0, 1), op1)
> && rtx_equal_p (XEXP (op0, 0), op2))
> return op2;
> + else if (! side_effects_p (op0))
> + {
> + rtx temp;
> + temp = simplify_relational_operation (GET_CODE (op0), op0_mode,
> + XEXP (op0, 0), XEXP (op0, 1));
We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1
is the 'then expr' and op2 is the 'else expr'.
The parameters of simplify_relational_operation have the following meaning:
...
/* Like simplify_binary_operation except used for relational operators.
MODE is the mode of the operands, not that of the result. If MODE
is VOIDmode, both operands must also be VOIDmode and we compare the
operands in "infinite precision".
If no simplification is possible, this function returns zero.
Otherwise, it returns either const_true_rtx or const0_rtx. */
rtx
simplify_relational_operation (code, mode, op0, op1)
enum rtx_code code;
enum machine_mode mode;
rtx op0, op1;
...
The problem in the patch is that we use op0_mode argument for the mode
parameter. The mode parameter of simplify_relational_operation needs to
be the mode of the operands of the condition, while op0_mode is the mode
of the condition.
Patch below fixes this on current trunk.
[ I found this by running into an ICE in
gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to
reproduce this with an upstream branch yet. ]
OK for trunk if bootstrap and reg-test for x86_64 succeeds?
Thanks,
- Tom
[-- Attachment #2: tmp.patch --]
[-- Type: text/x-patch, Size: 457 bytes --]
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 0133d43..fbf979b 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -5567,8 +5567,6 @@ simplify_ternary_operation (enum rtx_code code, machine_mode mode,
XEXP (op0, 0), XEXP (op0, 1));
}
- if (cmp_mode == VOIDmode)
- cmp_mode = op0_mode;
temp = simplify_relational_operation (GET_CODE (op0), op0_mode,
cmp_mode, XEXP (op0, 0),
XEXP (op0, 1));
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix bug in simplify_ternary_operation
2017-08-28 18:50 [PATCH] Fix bug in simplify_ternary_operation Tom de Vries
@ 2017-08-30 9:12 ` Tom de Vries
2017-08-31 23:37 ` Jeff Law
1 sibling, 0 replies; 7+ messages in thread
From: Tom de Vries @ 2017-08-30 9:12 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]
On 08/28/2017 08:26 PM, Tom de Vries wrote:
> Hi,
>
> I think I found a bug in r17465:
> ...
>> * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE
>> simplifications.
>>
>> diff --git a/gcc/cse.c b/gcc/cse.c
>> index e001597..3c27387 100644
>> --- a/gcc/cse.c
>> +++ b/gcc/cse.c
>> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode,
>> op0_mode, op0, op1, op2)
>
> Note: the parameters of simplify_ternary_operation have the following
> meaning:
> ...
> /* Simplify CODE, an operation with result mode MODE and three operands,
> OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became
> a constant. Return 0 if no simplifications is possible. */
>
> rtx
> simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
> enum rtx_code code;
> enum machine_mode mode, op0_mode;
> rtx op0, op1, op2;
> ...
>
>> && rtx_equal_p (XEXP (op0, 1), op1)
>> && rtx_equal_p (XEXP (op0, 0), op2))
>> return op2;
>> + else if (! side_effects_p (op0))
>> + {
>> + rtx temp;
>> + temp = simplify_relational_operation (GET_CODE (op0), op0_mode,
>> + XEXP (op0, 0), XEXP
>> (op0, 1));
>
> We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1
> is the 'then expr' and op2 is the 'else expr'.
>
> The parameters of simplify_relational_operation have the following meaning:
> ...
> /* Like simplify_binary_operation except used for relational operators.
> MODE is the mode of the operands, not that of the result. If MODE
> is VOIDmode, both operands must also be VOIDmode and we compare the
> operands in "infinite precision".
>
> If no simplification is possible, this function returns zero.
> Otherwise, it returns either const_true_rtx or const0_rtx. */
>
> rtx
> simplify_relational_operation (code, mode, op0, op1)
> enum rtx_code code;
> enum machine_mode mode;
> rtx op0, op1;
> ...
>
> The problem in the patch is that we use op0_mode argument for the mode
> parameter. The mode parameter of simplify_relational_operation needs to
> be the mode of the operands of the condition, while op0_mode is the mode
> of the condition.
>
> Patch below fixes this on current trunk.
>
> [ I found this by running into an ICE in
> gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to
> reproduce this with an upstream branch yet. ]
Filed as PR82020 - "ICE in decompose at rtl.h:2126" (
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82020 ).
>
> OK for trunk if bootstrap and reg-test for x86_64 succeeds?
bootstrap and reg-test for x86_64 done, no issues found.
Thanks,
- Tom
[ reposting patch with ChangeLog entry ]
[-- Attachment #2: 0001-Fix-comparison-mode-in-simplify_ternary_operation.patch --]
[-- Type: text/x-patch, Size: 746 bytes --]
Fix comparison mode in simplify_ternary_operation
2017-08-29 Tom de Vries <tom@codesourcery.com>
PR rtl-optimization/82020
* simplify-rtx.c (simplify_ternary_operation): Fix comparison mode of
IF_THEN_ELSE condition.
---
gcc/simplify-rtx.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 0133d43..fbf979b 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -5567,8 +5567,6 @@ simplify_ternary_operation (enum rtx_code code, machine_mode mode,
XEXP (op0, 0), XEXP (op0, 1));
}
- if (cmp_mode == VOIDmode)
- cmp_mode = op0_mode;
temp = simplify_relational_operation (GET_CODE (op0), op0_mode,
cmp_mode, XEXP (op0, 0),
XEXP (op0, 1));
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix bug in simplify_ternary_operation
2017-08-28 18:50 [PATCH] Fix bug in simplify_ternary_operation Tom de Vries
2017-08-30 9:12 ` Tom de Vries
@ 2017-08-31 23:37 ` Jeff Law
2017-09-01 8:51 ` Tom de Vries
1 sibling, 1 reply; 7+ messages in thread
From: Jeff Law @ 2017-08-31 23:37 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
On 08/28/2017 12:26 PM, Tom de Vries wrote:
> Hi,
>
> I think I found a bug in r17465:
> ...
>> * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE
>> simplifications.
>>
>> diff --git a/gcc/cse.c b/gcc/cse.c
>> index e001597..3c27387 100644
>> --- a/gcc/cse.c
>> +++ b/gcc/cse.c
>> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode,
>> op0_mode, op0, op1, op2)
>
> Note: the parameters of simplify_ternary_operation have the following
> meaning:
> ...
> /* Simplify CODE, an operation with result mode MODE and three operands,
> OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became
> a constant. Return 0 if no simplifications is possible. */
>
> rtx
> simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
> enum rtx_code code;
> enum machine_mode mode, op0_mode;
> rtx op0, op1, op2;
> ...
>
>> && rtx_equal_p (XEXP (op0, 1), op1)
>> && rtx_equal_p (XEXP (op0, 0), op2))
>> return op2;
>> + else if (! side_effects_p (op0))
>> + {
>> + rtx temp;
>> + temp = simplify_relational_operation (GET_CODE (op0), op0_mode,
>> + XEXP (op0, 0), XEXP
>> (op0, 1));
>
> We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1
> is the 'then expr' and op2 is the 'else expr'.
>
> The parameters of simplify_relational_operation have the following meaning:
> ...
> /* Like simplify_binary_operation except used for relational operators.
> MODE is the mode of the operands, not that of the result. If MODE
> is VOIDmode, both operands must also be VOIDmode and we compare the
> operands in "infinite precision".
>
> If no simplification is possible, this function returns zero.
> Otherwise, it returns either const_true_rtx or const0_rtx. */
>
> rtx
> simplify_relational_operation (code, mode, op0, op1)
> enum rtx_code code;
> enum machine_mode mode;
> rtx op0, op1;
> ...
>
> The problem in the patch is that we use op0_mode argument for the mode
> parameter. The mode parameter of simplify_relational_operation needs to
> be the mode of the operands of the condition, while op0_mode is the mode
> of the condition.
>
> Patch below fixes this on current trunk.
>
> [ I found this by running into an ICE in
> gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to
> reproduce this with an upstream branch yet. ]
>
> OK for trunk if bootstrap and reg-test for x86_64 succeeds?
So clearly setting cmp_mode to op0_mode is wrong. But we also have to
make sure that if cmp_mode is VOIDmode that either XEXP (op0, 0) has a
non-void mode or that XEXP (op0, 1) has a non-void mode, otherwise we're
likely to abort down in simplify_const_relational_operation.
ISTM a better fix is to return NULL_RTX if cmp_mode is VOIDmode and both
the sub-operations are VOIDmode as well.
Can you try that and verify that pr28776-2.c continues to work?
jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix bug in simplify_ternary_operation
2017-08-31 23:37 ` Jeff Law
@ 2017-09-01 8:51 ` Tom de Vries
2017-09-25 16:33 ` [PING][PATCH] " Tom de Vries
2017-11-20 3:12 ` [PATCH] " Jeff Law
0 siblings, 2 replies; 7+ messages in thread
From: Tom de Vries @ 2017-09-01 8:51 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On 08/31/2017 11:44 PM, Jeff Law wrote:
> On 08/28/2017 12:26 PM, Tom de Vries wrote:
>> Hi,
>>
>> I think I found a bug in r17465:
>> ...
>>> * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE
>>> simplifications.
>>>
>>> diff --git a/gcc/cse.c b/gcc/cse.c
>>> index e001597..3c27387 100644
>>> --- a/gcc/cse.c
>>> +++ b/gcc/cse.c
>>> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode,
>>> op0_mode, op0, op1, op2)
>>
>> Note: the parameters of simplify_ternary_operation have the following
>> meaning:
>> ...
>> /* Simplify CODE, an operation with result mode MODE and three operands,
>> OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became
>> a constant. Return 0 if no simplifications is possible. */
>>
>> rtx
>> simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
>> enum rtx_code code;
>> enum machine_mode mode, op0_mode;
>> rtx op0, op1, op2;
>> ...
>>
>>> && rtx_equal_p (XEXP (op0, 1), op1)
>>> && rtx_equal_p (XEXP (op0, 0), op2))
>>> return op2;
>>> + else if (! side_effects_p (op0))
>>> + {
>>> + rtx temp;
>>> + temp = simplify_relational_operation (GET_CODE (op0), op0_mode,
>>> + XEXP (op0, 0), XEXP
>>> (op0, 1));
>>
>> We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1
>> is the 'then expr' and op2 is the 'else expr'.
>>
>> The parameters of simplify_relational_operation have the following meaning:
>> ...
>> /* Like simplify_binary_operation except used for relational operators.
>> MODE is the mode of the operands, not that of the result. If MODE
>> is VOIDmode, both operands must also be VOIDmode and we compare the
>> operands in "infinite precision".
>>
>> If no simplification is possible, this function returns zero.
>> Otherwise, it returns either const_true_rtx or const0_rtx. */
>>
>> rtx
>> simplify_relational_operation (code, mode, op0, op1)
>> enum rtx_code code;
>> enum machine_mode mode;
>> rtx op0, op1;
>> ...
>>
>> The problem in the patch is that we use op0_mode argument for the mode
>> parameter. The mode parameter of simplify_relational_operation needs to
>> be the mode of the operands of the condition, while op0_mode is the mode
>> of the condition.
>>
>> Patch below fixes this on current trunk.
>>
>> [ I found this by running into an ICE in
>> gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to
>> reproduce this with an upstream branch yet. ]
>>
>> OK for trunk if bootstrap and reg-test for x86_64 succeeds?
> So clearly setting cmp_mode to op0_mode is wrong. But we also have to
> make sure that if cmp_mode is VOIDmode that either XEXP (op0, 0) has a
> non-void mode or that XEXP (op0, 1) has a non-void mode, otherwise we're
> likely to abort down in simplify_const_relational_operation.
>
You're referring to this assert:
...
/* Check if the given comparison (done in the given MODE) is actually
a tautology or a contradiction. If the mode is VOID_mode, the
comparison is done in "infinite precision". If no simplification
is possible, this function returns zero. Otherwise, it returns
either const_true_rtx or const0_rtx. */
rtx
simplify_const_relational_operation (enum rtx_code code,
machine_mode mode,
rtx op0, rtx op1)
{
...
gcc_assert (mode != VOIDmode
|| (GET_MODE (op0) == VOIDmode
&& GET_MODE (op1) == VOIDmode));
...
added by Honza:
...
* simplify-rtx.c (simplify_relational_operation): Verify that
mode == VOIDmode implies both operands to be VOIDmode.
...
In other words, rewriting the assert in more readable form:
...
#define BOOL_IMPLIES(a, b) (!(a) || (b))
gcc_assert (BOOL_IMPLIES (mode == VOIDmode,
(GET_MODE (op0) == VOIDmode
&& GET_MODE (op1) == VOIDmode)));
...
[ I'd be in favor of rewriting imply relations using a macro or some
such, I find it easier to understand. ]
Now, simplify_relational_operation starts like this:
...
rtx
simplify_relational_operation (enum rtx_code code, machine_mode mode,
machine_mode cmp_mode, rtx op0, rtx op1)
{
rtx tem, trueop0, trueop1;
if (cmp_mode == VOIDmode)
cmp_mode = GET_MODE (op0);
if (cmp_mode == VOIDmode)
cmp_mode = GET_MODE (op1);
tem = simplify_const_relational_operation (code, cmp_mode, op0, op1);
...
AFAIU, the cmp_mode ifs ensure that the assert in
simplify_const_relational_operation doesn't trigger.
> ISTM a better fix is to return NULL_RTX if cmp_mode is VOIDmode and both
> the sub-operations are VOIDmode as well.
>
I don't think we need that. simplify_const_relational_operation can
handle the situation that mode == VOIDmode && GET_MODE (op0) == VOIDmode
&& GET_MODE (op1) == VOIDmode.
Thanks,
- Tom
> Can you try that and verify that pr28776-2.c continues to work?
> jeff
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PING][PATCH] Fix bug in simplify_ternary_operation
2017-09-01 8:51 ` Tom de Vries
@ 2017-09-25 16:33 ` Tom de Vries
2017-11-20 3:12 ` [PATCH] " Jeff Law
1 sibling, 0 replies; 7+ messages in thread
From: Tom de Vries @ 2017-09-25 16:33 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On 09/01/2017 10:51 AM, Tom de Vries wrote:
> On 08/31/2017 11:44 PM, Jeff Law wrote:
>> On 08/28/2017 12:26 PM, Tom de Vries wrote:
>>> Hi,
>>>
>>> I think I found a bug in r17465:
>>> ...
>>>> Â Â Â Â Â Â Â * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE
>>>> Â Â Â Â Â Â Â simplifications.
>>>>
>>>> diff --git a/gcc/cse.c b/gcc/cse.c
>>>> index e001597..3c27387 100644
>>>> --- a/gcc/cse.c
>>>> +++ b/gcc/cse.c
>>>> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode,
>>>> op0_mode, op0, op1, op2)
>>>
>>> Note: the parameters of simplify_ternary_operation have the following
>>> meaning:
>>> ...
>>> /* Simplify CODE, an operation with result mode MODE and three operands,
>>>    OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became
>>>    a constant. Return 0 if no simplifications is possible. */
>>>
>>> rtx
>>> simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
>>> Â Â Â Â Â enum rtx_code code;
>>> Â Â Â Â Â enum machine_mode mode, op0_mode;
>>> Â Â Â Â Â rtx op0, op1, op2;
>>> ...
>>>
>>>> Â Â Â Â Â Â Â Â Â Â && rtx_equal_p (XEXP (op0, 1), op1)
>>>> Â Â Â Â Â Â Â Â Â Â && rtx_equal_p (XEXP (op0, 0), op2))
>>>> Â Â Â Â Â Â Â Â return op2;
>>>> +Â Â Â Â Â else if (! side_effects_p (op0))
>>>> +Â Â Â Â Â Â {
>>>> +Â Â Â Â Â Â Â Â rtx temp;
>>>> +Â Â Â Â Â Â Â Â temp = simplify_relational_operation (GET_CODE (op0),
>>>> op0_mode,
>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â XEXP (op0, 0), XEXP
>>>> (op0, 1));
>>>
>>> We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1
>>> is the 'then expr' and op2 is the 'else expr'.
>>>
>>> The parameters of simplify_relational_operation have the following
>>> meaning:
>>> ...
>>> /* Like simplify_binary_operation except used for relational operators.
>>>    MODE is the mode of the operands, not that of the result. If MODE
>>> Â Â Â is VOIDmode, both operands must also be VOIDmode and we compare the
>>> Â Â Â operands in "infinite precision".
>>>
>>> Â Â Â If no simplification is possible, this function returns zero.
>>>    Otherwise, it returns either const_true_rtx or const0_rtx. */
>>>
>>> rtx
>>> simplify_relational_operation (code, mode, op0, op1)
>>> Â Â Â Â Â enum rtx_code code;
>>> Â Â Â Â Â enum machine_mode mode;
>>> Â Â Â Â Â rtx op0, op1;
>>> ...
>>>
>>> The problem in the patch is that we use op0_mode argument for the mode
>>> parameter. The mode parameter of simplify_relational_operation needs to
>>> be the mode of the operands of the condition, while op0_mode is the mode
>>> of the condition.
>>>
>>> Patch below fixes this on current trunk.
>>>
>>> [ I found this by running into an ICE in
>>> gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to
>>> reproduce this with an upstream branch yet. ]
>>>
>>> OK for trunk if bootstrap and reg-test for x86_64 succeeds?
>> So clearly setting cmp_mode to op0_mode is wrong.  But we also have to
>> make sure that if cmp_mode is VOIDmode that either XEXP (op0, 0) has a
>> non-void mode or that XEXP (op0, 1) has a non-void mode, otherwise we're
>> likely to abort down in simplify_const_relational_operation.
>>
>
> You're referring to this assert:
> ...
> /* Check if the given comparison (done in the given MODE) is actually
>   a tautology or a contradiction. If the mode is VOID_mode, the
>   comparison is done in "infinite precision". If no simplification
>   is possible, this function returns zero. Otherwise, it returns
>   either const_true_rtx or const0_rtx. */
>
> rtx
> simplify_const_relational_operation (enum rtx_code code,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â machine_mode mode,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â rtx op0, rtx op1)
> {
> Â ...
>
> Â gcc_assert (mode != VOIDmode
> Â Â Â Â Â Â Â Â Â Â Â Â Â || (GET_MODE (op0) == VOIDmode
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && GET_MODE (op1) == VOIDmode));
> ...
>
> added by Honza:
> ...
> Â Â Â Â * simplify-rtx.c (simplify_relational_operation): Verify that
> Â Â Â Â Â Â Â mode == VOIDmode implies both operands to be VOIDmode.
> ...
>
> In other words, rewriting the assert in more readable form:
> ...
> #define BOOL_IMPLIES(a, b) (!(a) || (b))
> Â gcc_assert (BOOL_IMPLIES (mode == VOIDmode,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (GET_MODE (op0) == VOIDmode
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && GET_MODE (op1) == VOIDmode)));
> ...
> [ I'd be in favor of rewriting imply relations using a macro or some
> such, I find it easier to understand. ]
>
> Now, simplify_relational_operation starts like this:
> ...
> rtx
> simplify_relational_operation (enum rtx_code code, machine_mode mode,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â machine_mode cmp_mode, rtx op0, rtx op1)
> {
> Â rtx tem, trueop0, trueop1;
>
> Â if (cmp_mode == VOIDmode)
> Â Â Â cmp_mode = GET_MODE (op0);
> Â if (cmp_mode == VOIDmode)
> Â Â Â cmp_mode = GET_MODE (op1);
>
> Â tem = simplify_const_relational_operation (code, cmp_mode, op0, op1);
> ...
>
> AFAIU, the cmp_mode ifs ensure that the assert in
> simplify_const_relational_operation doesn't trigger.
>
>> ISTM a better fix is to return NULL_RTX if cmp_mode is VOIDmode and both
>> the sub-operations are VOIDmode as well.
>>
>
> I don't think we need that. simplify_const_relational_operation can
> handle the situation that mode == VOIDmode && GET_MODE (op0) == VOIDmode
> && GET_MODE (op1) == VOIDmode.
>
Ping.
Thanks,
- Tom
>> Can you try that and verify that pr28776-2.c continues to work?
>> jeff
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix bug in simplify_ternary_operation
2017-09-01 8:51 ` Tom de Vries
2017-09-25 16:33 ` [PING][PATCH] " Tom de Vries
@ 2017-11-20 3:12 ` Jeff Law
2017-11-20 9:48 ` Tom de Vries
1 sibling, 1 reply; 7+ messages in thread
From: Jeff Law @ 2017-11-20 3:12 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
Sorry, it's taken so long to get back to this patch...
On 09/01/2017 02:51 AM, Tom de Vries wrote:
> On 08/31/2017 11:44 PM, Jeff Law wrote:
>> On 08/28/2017 12:26 PM, Tom de Vries wrote:
>>> Hi,
>>>
>>> I think I found a bug in r17465:
>>> ...
>>>> Â Â Â Â Â Â Â * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE
>>>> Â Â Â Â Â Â Â simplifications.
>>>>
>>>> diff --git a/gcc/cse.c b/gcc/cse.c
>>>> index e001597..3c27387 100644
>>>> --- a/gcc/cse.c
>>>> +++ b/gcc/cse.c
>>>> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode,
>>>> op0_mode, op0, op1, op2)
>>>
>>> Note: the parameters of simplify_ternary_operation have the following
>>> meaning:
>>> ...
>>> /* Simplify CODE, an operation with result mode MODE and three operands,
>>>    OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became
>>>    a constant. Return 0 if no simplifications is possible. */
>>>
>>> rtx
>>> simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
>>> Â Â Â Â Â enum rtx_code code;
>>> Â Â Â Â Â enum machine_mode mode, op0_mode;
>>> Â Â Â Â Â rtx op0, op1, op2;
>>> ...
>>>
>>>> Â Â Â Â Â Â Â Â Â Â && rtx_equal_p (XEXP (op0, 1), op1)
>>>> Â Â Â Â Â Â Â Â Â Â && rtx_equal_p (XEXP (op0, 0), op2))
>>>> Â Â Â Â Â Â Â Â return op2;
>>>> +Â Â Â Â Â else if (! side_effects_p (op0))
>>>> +Â Â Â Â Â Â {
>>>> +Â Â Â Â Â Â Â Â rtx temp;
>>>> +Â Â Â Â Â Â Â Â temp = simplify_relational_operation (GET_CODE (op0),
>>>> op0_mode,
>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â XEXP (op0, 0), XEXP
>>>> (op0, 1));
>>>
>>> We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1
>>> is the 'then expr' and op2 is the 'else expr'.
>>>
>>> The parameters of simplify_relational_operation have the following
>>> meaning:
>>> ...
>>> /* Like simplify_binary_operation except used for relational operators.
>>>    MODE is the mode of the operands, not that of the result. If MODE
>>> Â Â Â is VOIDmode, both operands must also be VOIDmode and we compare the
>>> Â Â Â operands in "infinite precision".
>>>
>>> Â Â Â If no simplification is possible, this function returns zero.
>>>    Otherwise, it returns either const_true_rtx or const0_rtx. */
>>>
>>> rtx
>>> simplify_relational_operation (code, mode, op0, op1)
>>> Â Â Â Â Â enum rtx_code code;
>>> Â Â Â Â Â enum machine_mode mode;
>>> Â Â Â Â Â rtx op0, op1;
>>> ...
>>>
>>> The problem in the patch is that we use op0_mode argument for the mode
>>> parameter. The mode parameter of simplify_relational_operation needs to
>>> be the mode of the operands of the condition, while op0_mode is the mode
>>> of the condition.
>>>
>>> Patch below fixes this on current trunk.
>>>
>>> [ I found this by running into an ICE in
>>> gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to
>>> reproduce this with an upstream branch yet. ]
>>>
>>> OK for trunk if bootstrap and reg-test for x86_64 succeeds?
>> So clearly setting cmp_mode to op0_mode is wrong.  But we also have to
>> make sure that if cmp_mode is VOIDmode that either XEXP (op0, 0) has a
>> non-void mode or that XEXP (op0, 1) has a non-void mode, otherwise we're
>> likely to abort down in simplify_const_relational_operation.
>>
>
> You're referring to this assert:
> ...
> /* Check if the given comparison (done in the given MODE) is actually
>   a tautology or a contradiction. If the mode is VOID_mode, the
>   comparison is done in "infinite precision". If no simplification
>   is possible, this function returns zero. Otherwise, it returns
>   either const_true_rtx or const0_rtx. */
>
> rtx
> simplify_const_relational_operation (enum rtx_code code,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â machine_mode mode,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â rtx op0, rtx op1)
> {
> Â ...
>
> Â gcc_assert (mode != VOIDmode
> Â Â Â Â Â Â Â Â Â Â Â Â Â || (GET_MODE (op0) == VOIDmode
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && GET_MODE (op1) == VOIDmode));
> ...
>
> added by Honza:
> ...
> Â Â Â Â * simplify-rtx.c (simplify_relational_operation): Verify that
> Â Â Â Â Â Â Â mode == VOIDmode implies both operands to be VOIDmode.
> ...
>
> In other words, rewriting the assert in more readable form:
> ...
> #define BOOL_IMPLIES(a, b) (!(a) || (b))
> Â gcc_assert (BOOL_IMPLIES (mode == VOIDmode,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (GET_MODE (op0) == VOIDmode
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && GET_MODE (op1) == VOIDmode)));
> ...
> [ I'd be in favor of rewriting imply relations using a macro or some
> such, I find it easier to understand. ]
>
> Now, simplify_relational_operation starts like this:
> ...
> rtx
> simplify_relational_operation (enum rtx_code code, machine_mode mode,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â machine_mode cmp_mode, rtx op0, rtx op1)
> {
> Â rtx tem, trueop0, trueop1;
>
> Â if (cmp_mode == VOIDmode)
> Â Â Â cmp_mode = GET_MODE (op0);
> Â if (cmp_mode == VOIDmode)
> Â Â Â cmp_mode = GET_MODE (op1);
>
> Â tem = simplify_const_relational_operation (code, cmp_mode, op0, op1);
> ...
>
> AFAIU, the cmp_mode ifs ensure that the assert in
> simplify_const_relational_operation doesn't trigger.
>
>> ISTM a better fix is to return NULL_RTX if cmp_mode is VOIDmode and both
>> the sub-operations are VOIDmode as well.
>>
>
> I don't think we need that. simplify_const_relational_operation can
> handle the situation that mode == VOIDmode && GET_MODE (op0) == VOIDmode
> && GET_MODE (op1) == VOIDmode.
I think you're right -- looking back at it again I think I mis-read the
assert.
Go ahead and commit your change.
Thanks again for your patience.
jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix bug in simplify_ternary_operation
2017-11-20 3:12 ` [PATCH] " Jeff Law
@ 2017-11-20 9:48 ` Tom de Vries
0 siblings, 0 replies; 7+ messages in thread
From: Tom de Vries @ 2017-11-20 9:48 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On 11/20/2017 03:52 AM, Jeff Law wrote:
> Sorry, it's taken so long to get back to this patch...
>
>
> On 09/01/2017 02:51 AM, Tom de Vries wrote:
>> On 08/31/2017 11:44 PM, Jeff Law wrote:
>>> On 08/28/2017 12:26 PM, Tom de Vries wrote:
>>>> Hi,
>>>>
>>>> I think I found a bug in r17465:
>>>> ...
>>>>> Â Â Â Â Â Â Â * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE
>>>>> Â Â Â Â Â Â Â simplifications.
>>>>>
>>>>> diff --git a/gcc/cse.c b/gcc/cse.c
>>>>> index e001597..3c27387 100644
>>>>> --- a/gcc/cse.c
>>>>> +++ b/gcc/cse.c
>>>>> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode,
>>>>> op0_mode, op0, op1, op2)
>>>>
>>>> Note: the parameters of simplify_ternary_operation have the following
>>>> meaning:
>>>> ...
>>>> /* Simplify CODE, an operation with result mode MODE and three operands,
>>>>    OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became
>>>>    a constant. Return 0 if no simplifications is possible. */
>>>>
>>>> rtx
>>>> simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2)
>>>> Â Â Â Â Â enum rtx_code code;
>>>> Â Â Â Â Â enum machine_mode mode, op0_mode;
>>>> Â Â Â Â Â rtx op0, op1, op2;
>>>> ...
>>>>
>>>>> Â Â Â Â Â Â Â Â Â Â && rtx_equal_p (XEXP (op0, 1), op1)
>>>>> Â Â Â Â Â Â Â Â Â Â && rtx_equal_p (XEXP (op0, 0), op2))
>>>>> Â Â Â Â Â Â Â Â return op2;
>>>>> +Â Â Â Â Â else if (! side_effects_p (op0))
>>>>> +Â Â Â Â Â Â {
>>>>> +Â Â Â Â Â Â Â Â rtx temp;
>>>>> +Â Â Â Â Â Â Â Â temp = simplify_relational_operation (GET_CODE (op0),
>>>>> op0_mode,
>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â XEXP (op0, 0), XEXP
>>>>> (op0, 1));
>>>>
>>>> We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1
>>>> is the 'then expr' and op2 is the 'else expr'.
>>>>
>>>> The parameters of simplify_relational_operation have the following
>>>> meaning:
>>>> ...
>>>> /* Like simplify_binary_operation except used for relational operators.
>>>>    MODE is the mode of the operands, not that of the result. If MODE
>>>> Â Â Â is VOIDmode, both operands must also be VOIDmode and we compare the
>>>> Â Â Â operands in "infinite precision".
>>>>
>>>> Â Â Â If no simplification is possible, this function returns zero.
>>>>    Otherwise, it returns either const_true_rtx or const0_rtx. */
>>>>
>>>> rtx
>>>> simplify_relational_operation (code, mode, op0, op1)
>>>> Â Â Â Â Â enum rtx_code code;
>>>> Â Â Â Â Â enum machine_mode mode;
>>>> Â Â Â Â Â rtx op0, op1;
>>>> ...
>>>>
>>>> The problem in the patch is that we use op0_mode argument for the mode
>>>> parameter. The mode parameter of simplify_relational_operation needs to
>>>> be the mode of the operands of the condition, while op0_mode is the mode
>>>> of the condition.
>>>>
>>>> Patch below fixes this on current trunk.
>>>>
>>>> [ I found this by running into an ICE in
>>>> gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to
>>>> reproduce this with an upstream branch yet. ]
>>>>
>>>> OK for trunk if bootstrap and reg-test for x86_64 succeeds?
>>> So clearly setting cmp_mode to op0_mode is wrong.  But we also have to
>>> make sure that if cmp_mode is VOIDmode that either XEXP (op0, 0) has a
>>> non-void mode or that XEXP (op0, 1) has a non-void mode, otherwise we're
>>> likely to abort down in simplify_const_relational_operation.
>>>
>>
>> You're referring to this assert:
>> ...
>> /* Check if the given comparison (done in the given MODE) is actually
>>   a tautology or a contradiction. If the mode is VOID_mode, the
>>   comparison is done in "infinite precision". If no simplification
>>   is possible, this function returns zero. Otherwise, it returns
>>   either const_true_rtx or const0_rtx. */
>>
>> rtx
>> simplify_const_relational_operation (enum rtx_code code,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â machine_mode mode,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â rtx op0, rtx op1)
>> {
>> Â ...
>>
>> Â gcc_assert (mode != VOIDmode
>> Â Â Â Â Â Â Â Â Â Â Â Â Â || (GET_MODE (op0) == VOIDmode
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && GET_MODE (op1) == VOIDmode));
>> ...
>>
>> added by Honza:
>> ...
>> Â Â Â Â * simplify-rtx.c (simplify_relational_operation): Verify that
>> Â Â Â Â Â Â Â mode == VOIDmode implies both operands to be VOIDmode.
>> ...
>>
>> In other words, rewriting the assert in more readable form:
>> ...
>> #define BOOL_IMPLIES(a, b) (!(a) || (b))
>> Â gcc_assert (BOOL_IMPLIES (mode == VOIDmode,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (GET_MODE (op0) == VOIDmode
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && GET_MODE (op1) == VOIDmode)));
>> ...
>> [ I'd be in favor of rewriting imply relations using a macro or some
>> such, I find it easier to understand. ]
>>
>> Now, simplify_relational_operation starts like this:
>> ...
>> rtx
>> simplify_relational_operation (enum rtx_code code, machine_mode mode,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â machine_mode cmp_mode, rtx op0, rtx op1)
>> {
>> Â rtx tem, trueop0, trueop1;
>>
>> Â if (cmp_mode == VOIDmode)
>> Â Â Â cmp_mode = GET_MODE (op0);
>> Â if (cmp_mode == VOIDmode)
>> Â Â Â cmp_mode = GET_MODE (op1);
>>
>> Â tem = simplify_const_relational_operation (code, cmp_mode, op0, op1);
>> ...
>>
>> AFAIU, the cmp_mode ifs ensure that the assert in
>> simplify_const_relational_operation doesn't trigger.
>>
>>> ISTM a better fix is to return NULL_RTX if cmp_mode is VOIDmode and both
>>> the sub-operations are VOIDmode as well.
>>>
>>
>> I don't think we need that. simplify_const_relational_operation can
>> handle the situation that mode == VOIDmode && GET_MODE (op0) == VOIDmode
>> && GET_MODE (op1) == VOIDmode.
> I think you're right -- looking back at it again I think I mis-read the
> assert.
>
> Go ahead and commit your change.
Done.
> Thanks again for your patience.
>
No problem, and thanks for the review :) .
- Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-20 8:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-28 18:50 [PATCH] Fix bug in simplify_ternary_operation Tom de Vries
2017-08-30 9:12 ` Tom de Vries
2017-08-31 23:37 ` Jeff Law
2017-09-01 8:51 ` Tom de Vries
2017-09-25 16:33 ` [PING][PATCH] " Tom de Vries
2017-11-20 3:12 ` [PATCH] " Jeff Law
2017-11-20 9:48 ` Tom de Vries
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).