[PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

Hi all,

In this PR we get an ICE when trying to emit a conditional move through noce_convert_multiple_sets.
The comment in the patch explains the situation but we get a two-instruction sequence like:
(insn 20 19 21 3 (set (reg:SI 89 [ _5 ])
         (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal}
      (nil))
(insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ])
         (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal}
      (nil))

The first instruction feeds the second, but the second takes the lowpart subreg of the first destination.
This leads to the noce_emit_cmove call taking as arguments the first SImode destination and the second HImode
destination.  This causes an assertion failure deeper down the line.

The solution in this patch is catch this case and wrap the first destination in a lowpart subreg so that the two
operands of the cmove have the same mode.

Bootstrapped and tested on aarch64-none-linux-gnu, arm-none-linux-gnueabihf and x86_64-unknown-linux-gnu.

Ok for trunk?

Thanks,
Kyrill

2016-06-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/71594
     * ifcvt.c (noce_convert_multiple_sets): Wrap new_val or old_val
     into subregs of appropriate mode before trying to emit a conditional
     move.

2016-06-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/71594
     * gcc.dg/torture/pr71594.c: New test.

[-- Attachment #2: ifcvt-modes.patch --]
[-- Type: text/x-patch, Size: 2197 bytes --]

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index fd2951673fb6bd6d9e5d52cdb88765434a603fb6..3617de674561019a8dd155f13391c6eb3cbac1e4 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3228,6 +3228,33 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
       if (if_info->then_else_reversed)
 	std::swap (old_val, new_val);
 
+
+      /* We allow simple lowpart register subreg SET sources in
+	 bb_ok_for_noce_convert_multiple_sets.  Be careful when processing
+	 sequences like:
+	 (set (reg:SI r1) (reg:SI r2))
+	 (set (reg:HI r3) (subreg:HI (r1)))
+	 For the second insn new_val or old_val (r1 in this example) will be
+	 taken from the temporaries and have the wider mode which will not
+	 match with the mode of the other source of the conditional move, so
+	 we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).
+	 Wrap the two cmove operands into subregs if appropriate to prevent
+	 that.  */
+      if (GET_MODE (new_val) != GET_MODE (temp))
+	{
+	  machine_mode src_mode = GET_MODE (new_val);
+	  machine_mode dst_mode = GET_MODE (temp);
+	  gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode));
+	  new_val = lowpart_subreg (dst_mode, new_val, src_mode);
+	}
+      if (GET_MODE (old_val) != GET_MODE (temp))
+	{
+	  machine_mode src_mode = GET_MODE (old_val);
+	  machine_mode dst_mode = GET_MODE (temp);
+	  gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode));
+	  old_val = lowpart_subreg (dst_mode, old_val, src_mode);
+	}
+
       /* Actually emit the conditional move.  */
       rtx temp_dest = noce_emit_cmove (if_info, temp, cond_code,
 				       x, y, new_val, old_val);
diff --git a/gcc/testsuite/gcc.dg/torture/pr71594.c b/gcc/testsuite/gcc.dg/torture/pr71594.c
new file mode 100644
index 0000000000000000000000000000000000000000..468a9f6891c92ff76520af0eee242f08b01ae0cf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr71594.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "--param max-rtl-if-conversion-insns=2" } */
+
+unsigned short a;
+int b, c;
+int *d;
+void fn1() {
+  *d = 24;
+  for (; *d <= 65;) {
+    unsigned short *e = &a;
+    b = (a &= 0 <= 0) < (c ?: (*e %= *d));
+    for (; *d <= 83;)
+      ;
+  }
+}

Re: [PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01731.html

Thanks,
Kyrill

On 24/06/16 09:32, Kyrill Tkachov wrote:
> Hi all,
>
> In this PR we get an ICE when trying to emit a conditional move through noce_convert_multiple_sets.
> The comment in the patch explains the situation but we get a two-instruction sequence like:
> (insn 20 19 21 3 (set (reg:SI 89 [ _5 ])
>         (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal}
>      (nil))
> (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ])
>         (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal}
>      (nil))
>
> The first instruction feeds the second, but the second takes the lowpart subreg of the first destination.
> This leads to the noce_emit_cmove call taking as arguments the first SImode destination and the second HImode
> destination.  This causes an assertion failure deeper down the line.
>
> The solution in this patch is catch this case and wrap the first destination in a lowpart subreg so that the two
> operands of the cmove have the same mode.
>
> Bootstrapped and tested on aarch64-none-linux-gnu, arm-none-linux-gnueabihf and x86_64-unknown-linux-gnu.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> 2016-06-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR rtl-optimization/71594
>     * ifcvt.c (noce_convert_multiple_sets): Wrap new_val or old_val
>     into subregs of appropriate mode before trying to emit a conditional
>     move.
>
> 2016-06-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR rtl-optimization/71594
>     * gcc.dg/torture/pr71594.c: New test.

Re: [PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Bernd Schmidt]

On 07/04/2016 12:28 PM, Kyrill Tkachov wrote:
> Ping.
> https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01731.html
>
> Thanks,
> Kyrill
>
> On 24/06/16 09:32, Kyrill Tkachov wrote:
>> Hi all,
>>
>> In this PR we get an ICE when trying to emit a conditional move
>> through noce_convert_multiple_sets.
>> The comment in the patch explains the situation but we get a
>> two-instruction sequence like:
>> (insn 20 19 21 3 (set (reg:SI 89 [ _5 ])
>>         (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal}
>>      (nil))
>> (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ])
>>         (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal}
>>      (nil))

> +
> +      /* We allow simple lowpart register subreg SET sources in
> +	 bb_ok_for_noce_convert_multiple_sets.  Be careful when processing
> +	 sequences like:
> +	 (set (reg:SI r1) (reg:SI r2))
> +	 (set (reg:HI r3) (subreg:HI (r1)))
> +	 For the second insn new_val or old_val (r1 in this example) will be
> +	 taken from the temporaries and have the wider mode which will not
> +	 match with the mode of the other source of the conditional move, so
> +	 we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).
> +	 Wrap the two cmove operands into subregs if appropriate to prevent
> +	 that.  */
> +      if (GET_MODE (new_val) != GET_MODE (temp))
> +	{
> +	  machine_mode src_mode = GET_MODE (new_val);
> +	  machine_mode dst_mode = GET_MODE (temp);
> +	  gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode));
> +	  new_val = lowpart_subreg (dst_mode, new_val, src_mode);

The question I have would be what happens if you have the inverse of the 
sequence you expect, maybe with multi-word regs?

(set (reg:SI 0) (reg:SI x))
(set (reg:DI y) (reg:DI 0))

That seems like it would fail the assert. Maybe this is something we 
need to catch in the bb_ok function.


Bernd

Re: [PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Kyrill Tkachov]

Hi Bernd,

On 04/07/16 12:08, Bernd Schmidt wrote:
>
>
> On 07/04/2016 12:28 PM, Kyrill Tkachov wrote:
>> Ping.
>> https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01731.html
>>
>> Thanks,
>> Kyrill
>>
>> On 24/06/16 09:32, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> In this PR we get an ICE when trying to emit a conditional move
>>> through noce_convert_multiple_sets.
>>> The comment in the patch explains the situation but we get a
>>> two-instruction sequence like:
>>> (insn 20 19 21 3 (set (reg:SI 89 [ _5 ])
>>>         (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal}
>>>      (nil))
>>> (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ])
>>>         (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal}
>>>      (nil))
>
>> +
>> +      /* We allow simple lowpart register subreg SET sources in
>> +     bb_ok_for_noce_convert_multiple_sets.  Be careful when processing
>> +     sequences like:
>> +     (set (reg:SI r1) (reg:SI r2))
>> +     (set (reg:HI r3) (subreg:HI (r1)))
>> +     For the second insn new_val or old_val (r1 in this example) will be
>> +     taken from the temporaries and have the wider mode which will not
>> +     match with the mode of the other source of the conditional move, so
>> +     we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).
>> +     Wrap the two cmove operands into subregs if appropriate to prevent
>> +     that.  */
>> +      if (GET_MODE (new_val) != GET_MODE (temp))
>> +    {
>> +      machine_mode src_mode = GET_MODE (new_val);
>> +      machine_mode dst_mode = GET_MODE (temp);
>> +      gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode));
>> +      new_val = lowpart_subreg (dst_mode, new_val, src_mode);
>
> The question I have would be what happens if you have the inverse of the sequence you expect, maybe with multi-word regs?
>
> (set (reg:SI 0) (reg:SI x))
> (set (reg:DI y) (reg:DI 0))
>
> That seems like it would fail the assert. Maybe this is something we need to catch in the bb_ok function.
>

That does seem like it could cause trouble but I couldn't think of how that sequence could appear or what its
semantics would be. Would assigning to the SImode reg 0 in your example not touch the upper bits of the DImode
value?

In any case, bb_ok_for_noce_convert_multiple_sets doesn't keep track of dependencies between the instructions
so I think the best place to handle this case would be in noce_convert_multiple_sets where instead of the assert
in this patch we'd just end_sequence () and return FALSE.
Would that be preferable?

Thanks for the review,
Kyrill


>
> Bernd
>

Re: [PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Bernd Schmidt]

On 07/04/2016 01:18 PM, Kyrill Tkachov wrote:
> That does seem like it could cause trouble but I couldn't think of how
> that sequence could appear or what its
> semantics would be. Would assigning to the SImode reg 0 in your example
> not touch the upper bits of the DImode value?

No, multi-word subreg accesses are per-word.

> In any case, bb_ok_for_noce_convert_multiple_sets doesn't keep track of
> dependencies between the instructions
> so I think the best place to handle this case would be in
> noce_convert_multiple_sets where instead of the assert
> in this patch we'd just end_sequence () and return FALSE.
> Would that be preferable?

That should at least work, and I'd be ok with that.


Bernd

Re: [PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]


On 04/07/16 12:19, Bernd Schmidt wrote:
> On 07/04/2016 01:18 PM, Kyrill Tkachov wrote:
>> That does seem like it could cause trouble but I couldn't think of how
>> that sequence could appear or what its
>> semantics would be. Would assigning to the SImode reg 0 in your example
>> not touch the upper bits of the DImode value?
>
> No, multi-word subreg accesses are per-word.
>
>> In any case, bb_ok_for_noce_convert_multiple_sets doesn't keep track of
>> dependencies between the instructions
>> so I think the best place to handle this case would be in
>> noce_convert_multiple_sets where instead of the assert
>> in this patch we'd just end_sequence () and return FALSE.
>> Would that be preferable?
>
> That should at least work, and I'd be ok with that.
>

Ok, here's the updated patch with the assert replaced by failing the conversion.
Bootstrapped and tested on x86_64. Also tested on aarch64.

Is this ok?

Thanks,
Kyrill

2016-07-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/71594
     * ifcvt.c (noce_convert_multiple_sets): Wrap new_val or old_val
     into subregs of appropriate mode before trying to emit a conditional
     move.

2016-07-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/71594
     * gcc.dg/torture/pr71594.c: New test.

[-- Attachment #2: ifcvt-modes.patch --]
[-- Type: text/x-patch, Size: 2307 bytes --]

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index fd2951673fb6bd6d9e5d52cdb88765434a603fb6..f7f120e04b11dc4f25be969e0c183a36e067a61c 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3228,6 +3228,41 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
       if (if_info->then_else_reversed)
 	std::swap (old_val, new_val);
 
+
+      /* We allow simple lowpart register subreg SET sources in
+	 bb_ok_for_noce_convert_multiple_sets.  Be careful when processing
+	 sequences like:
+	 (set (reg:SI r1) (reg:SI r2))
+	 (set (reg:HI r3) (subreg:HI (r1)))
+	 For the second insn new_val or old_val (r1 in this example) will be
+	 taken from the temporaries and have the wider mode which will not
+	 match with the mode of the other source of the conditional move, so
+	 we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).
+	 Wrap the two cmove operands into subregs if appropriate to prevent
+	 that.  */
+      if (GET_MODE (new_val) != GET_MODE (temp))
+	{
+	  machine_mode src_mode = GET_MODE (new_val);
+	  machine_mode dst_mode = GET_MODE (temp);
+	  if (GET_MODE_SIZE (src_mode) <= GET_MODE_SIZE (dst_mode))
+	    {
+	      end_sequence ();
+	      return FALSE;
+	    }
+	  new_val = lowpart_subreg (dst_mode, new_val, src_mode);
+	}
+      if (GET_MODE (old_val) != GET_MODE (temp))
+	{
+	  machine_mode src_mode = GET_MODE (old_val);
+	  machine_mode dst_mode = GET_MODE (temp);
+	  if (GET_MODE_SIZE (src_mode) <= GET_MODE_SIZE (dst_mode))
+	    {
+	      end_sequence ();
+	      return FALSE;
+	    }
+	  old_val = lowpart_subreg (dst_mode, old_val, src_mode);
+	}
+
       /* Actually emit the conditional move.  */
       rtx temp_dest = noce_emit_cmove (if_info, temp, cond_code,
 				       x, y, new_val, old_val);
diff --git a/gcc/testsuite/gcc.dg/torture/pr71594.c b/gcc/testsuite/gcc.dg/torture/pr71594.c
new file mode 100644
index 0000000000000000000000000000000000000000..468a9f6891c92ff76520af0eee242f08b01ae0cf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr71594.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "--param max-rtl-if-conversion-insns=2" } */
+
+unsigned short a;
+int b, c;
+int *d;
+void fn1() {
+  *d = 24;
+  for (; *d <= 65;) {
+    unsigned short *e = &a;
+    b = (a &= 0 <= 0) < (c ?: (*e %= *d));
+    for (; *d <= 83;)
+      ;
+  }
+}

Re: [PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Bernd Schmidt]

On 07/05/2016 03:50 PM, Kyrill Tkachov wrote:
> Ok, here's the updated patch with the assert replaced by failing the
> conversion.
> Bootstrapped and tested on x86_64. Also tested on aarch64.
>
> Is this ok?

Sure. Thanks!


Bernd