[PATCH] Fix bug in simplify_ternary_operation
Tom de Vries
Tom_deVries@mentor.com
Fri Sep 1 08:51:00 GMT 2017
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
>
More information about the Gcc-patches
mailing list