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
