| Message ID | 82ea4bb9-15cb-b00d-c6af-e1de926a9cec@mentor.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix bug in simplify_ternary_operation | expand |
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 ] 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));
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
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 >
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 >>
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
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
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));