[PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Andrew Pinski]

The problem is I used expand_expr with the target but
we don't want to use the target here as it is the wrong
mode for the original expression. The testcase would ICE
deap down while trying to do a move to use the target.
Anyways just calling expand_expr with NULL_EXPR fixes
the issue.

Committed as obvious after a bootstrap/test on x86_64-linux-gnu.

	PR middle-end/109919

gcc/ChangeLog:

	* expr.cc (expand_single_bit_test): Don't use the
	target for expand_expr.

gcc/testsuite/ChangeLog:

	* gcc.c-torture/compile/pr109919-1.c: New test.
---
 gcc/expr.cc                                      | 2 +-
 gcc/testsuite/gcc.c-torture/compile/pr109919-1.c | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr109919-1.c

diff --git a/gcc/expr.cc b/gcc/expr.cc
index 2070198acda..02f24c00148 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -12956,7 +12956,7 @@ expand_single_bit_test (location_t loc, enum tree_code code,
   intermediate_type = ops_unsigned ? unsigned_type : signed_type;
   inner = fold_convert_loc (loc, intermediate_type, inner);
 
-  rtx inner0 = expand_expr (inner, target, VOIDmode, EXPAND_NORMAL);
+  rtx inner0 = expand_expr (inner, NULL_RTX, VOIDmode, EXPAND_NORMAL);
 
   inner0 = extract_bit_field (inner0, 1, bitnum, 1, target,
 			      operand_mode, mode, 0, NULL);
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr109919-1.c b/gcc/testsuite/gcc.c-torture/compile/pr109919-1.c
new file mode 100644
index 00000000000..bb612a1f020
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr109919-1.c
@@ -0,0 +1,9 @@
+/* { dg-options "-fno-tree-dce -fno-tree-vrp" } */
+int a;
+int main() {
+  int b = 1;
+  while (a) {
+    short c = b && ((a || a) & (a * c));
+  }
+  return 0;
+}
-- 
2.31.1

Re: [PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Jeff Law]

On 5/20/23 19:09, Andrew Pinski via Gcc-patches wrote:
> The problem is I used expand_expr with the target but
> we don't want to use the target here as it is the wrong
> mode for the original expression. The testcase would ICE
> deap down while trying to do a move to use the target.
> Anyways just calling expand_expr with NULL_EXPR fixes
> the issue.
> 
> Committed as obvious after a bootstrap/test on x86_64-linux-gnu.
> 
> 	PR middle-end/109919
> 
> gcc/ChangeLog:
> 
> 	* expr.cc (expand_single_bit_test): Don't use the
> 	target for expand_expr.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.c-torture/compile/pr109919-1.c: New test.
Thanks.  I'll respin the targets that failed.  If you don't hear from 
me, assume everything is happy again after this fix.
jeff

Re: [PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Andrew Pinski]

On Sat, May 20, 2023 at 6:26 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 5/20/23 19:09, Andrew Pinski via Gcc-patches wrote:
> > The problem is I used expand_expr with the target but
> > we don't want to use the target here as it is the wrong
> > mode for the original expression. The testcase would ICE
> > deap down while trying to do a move to use the target.
> > Anyways just calling expand_expr with NULL_EXPR fixes
> > the issue.
> >
> > Committed as obvious after a bootstrap/test on x86_64-linux-gnu.
> >
> >       PR middle-end/109919
> >
> > gcc/ChangeLog:
> >
> >       * expr.cc (expand_single_bit_test): Don't use the
> >       target for expand_expr.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * gcc.c-torture/compile/pr109919-1.c: New test.
> Thanks.  I'll respin the targets that failed.  If you don't hear from
> me, assume everything is happy again after this fix.

Oh, I am going to test on aarch64-linux-gnu too just in case.
Expand is definitely something which I am not used to working on so I
figured I had made a mistake somewhere. I suspect I still made a
similar mistake later on too.

Thanks,
Andrew

> jeff

Re: [PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Jeff Law]

On 5/20/23 21:05, Andrew Pinski wrote:
> On Sat, May 20, 2023 at 6:26 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 5/20/23 19:09, Andrew Pinski via Gcc-patches wrote:
>>> The problem is I used expand_expr with the target but
>>> we don't want to use the target here as it is the wrong
>>> mode for the original expression. The testcase would ICE
>>> deap down while trying to do a move to use the target.
>>> Anyways just calling expand_expr with NULL_EXPR fixes
>>> the issue.
>>>
>>> Committed as obvious after a bootstrap/test on x86_64-linux-gnu.
>>>
>>>        PR middle-end/109919
>>>
>>> gcc/ChangeLog:
>>>
>>>        * expr.cc (expand_single_bit_test): Don't use the
>>>        target for expand_expr.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>        * gcc.c-torture/compile/pr109919-1.c: New test.
>> Thanks.  I'll respin the targets that failed.  If you don't hear from
>> me, assume everything is happy again after this fix.
> 
> Oh, I am going to test on aarch64-linux-gnu too just in case.
> Expand is definitely something which I am not used to working on so I
> figured I had made a mistake somewhere. I suspect I still made a
> similar mistake later on too.
I'm seeing some execution failures.  Building H8 bits now to debug as 
it's the target I'm most familiar with.   More info as it's available.

jeff

Re: [PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Andrew Pinski]

On Sat, May 20, 2023 at 8:25 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 5/20/23 21:05, Andrew Pinski wrote:
> > On Sat, May 20, 2023 at 6:26 PM Jeff Law via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >>
> >>
> >> On 5/20/23 19:09, Andrew Pinski via Gcc-patches wrote:
> >>> The problem is I used expand_expr with the target but
> >>> we don't want to use the target here as it is the wrong
> >>> mode for the original expression. The testcase would ICE
> >>> deap down while trying to do a move to use the target.
> >>> Anyways just calling expand_expr with NULL_EXPR fixes
> >>> the issue.
> >>>
> >>> Committed as obvious after a bootstrap/test on x86_64-linux-gnu.
> >>>
> >>>        PR middle-end/109919
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>>        * expr.cc (expand_single_bit_test): Don't use the
> >>>        target for expand_expr.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>>        * gcc.c-torture/compile/pr109919-1.c: New test.
> >> Thanks.  I'll respin the targets that failed.  If you don't hear from
> >> me, assume everything is happy again after this fix.
> >
> > Oh, I am going to test on aarch64-linux-gnu too just in case.
> > Expand is definitely something which I am not used to working on so I
> > figured I had made a mistake somewhere. I suspect I still made a
> > similar mistake later on too.
> I'm seeing some execution failures.  Building H8 bits now to debug as
> it's the target I'm most familiar with.   More info as it's available.

Is H8 big-endian? I could have messed that up.

Thanks,
Andrew Pinski

>
> jeff

Re: [PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Andrew Pinski]

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

On Sat, May 20, 2023 at 8:28 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Sat, May 20, 2023 at 8:25 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> >
> >
> >
> > On 5/20/23 21:05, Andrew Pinski wrote:
> > > On Sat, May 20, 2023 at 6:26 PM Jeff Law via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > >>
> > >>
> > >>
> > >> On 5/20/23 19:09, Andrew Pinski via Gcc-patches wrote:
> > >>> The problem is I used expand_expr with the target but
> > >>> we don't want to use the target here as it is the wrong
> > >>> mode for the original expression. The testcase would ICE
> > >>> deap down while trying to do a move to use the target.
> > >>> Anyways just calling expand_expr with NULL_EXPR fixes
> > >>> the issue.
> > >>>
> > >>> Committed as obvious after a bootstrap/test on x86_64-linux-gnu.
> > >>>
> > >>>        PR middle-end/109919
> > >>>
> > >>> gcc/ChangeLog:
> > >>>
> > >>>        * expr.cc (expand_single_bit_test): Don't use the
> > >>>        target for expand_expr.
> > >>>
> > >>> gcc/testsuite/ChangeLog:
> > >>>
> > >>>        * gcc.c-torture/compile/pr109919-1.c: New test.
> > >> Thanks.  I'll respin the targets that failed.  If you don't hear from
> > >> me, assume everything is happy again after this fix.
> > >
> > > Oh, I am going to test on aarch64-linux-gnu too just in case.
> > > Expand is definitely something which I am not used to working on so I
> > > figured I had made a mistake somewhere. I suspect I still made a
> > > similar mistake later on too.
> > I'm seeing some execution failures.  Building H8 bits now to debug as
> > it's the target I'm most familiar with.   More info as it's available.
>
> Is H8 big-endian? I could have messed that up.

If so, try the attached patch. I thought extract_bit_field's bitnum
field was like a shift and not like how BIT_FIELD_REF is defined.

Thanks,
Andrew

>
> Thanks,
> Andrew Pinski
>
> >
> > jeff

[-- Attachment #2: maybefixbigendian.diff.txt --]
[-- Type: text/plain, Size: 570 bytes --]

diff --git a/gcc/expr.cc b/gcc/expr.cc
index 02f24c00148..c033761f317 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -12958,7 +12958,12 @@ expand_single_bit_test (location_t loc, enum tree_code code,
 
   rtx inner0 = expand_expr (inner, NULL_RTX, VOIDmode, EXPAND_NORMAL);
 
-  inner0 = extract_bit_field (inner0, 1, bitnum, 1, target,
+  int bitpos = bitnum;
+
+  if (BYTES_BIG_ENDIAN)
+    bitpos = GET_MODE_BITSIZE (inner0) - 1 - bitpos;
+
+  inner0 = extract_bit_field (inner0, 1, bitpos, 1, target,
 			      operand_mode, mode, 0, NULL);
 
   if (code == EQ_EXPR)

Re: [PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Andrew Pinski]

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

On Sat, May 20, 2023 at 8:32 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Sat, May 20, 2023 at 8:28 PM Andrew Pinski <pinskia@gmail.com> wrote:
> >
> > On Sat, May 20, 2023 at 8:25 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> > >
> > >
> > >
> > > On 5/20/23 21:05, Andrew Pinski wrote:
> > > > On Sat, May 20, 2023 at 6:26 PM Jeff Law via Gcc-patches
> > > > <gcc-patches@gcc.gnu.org> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 5/20/23 19:09, Andrew Pinski via Gcc-patches wrote:
> > > >>> The problem is I used expand_expr with the target but
> > > >>> we don't want to use the target here as it is the wrong
> > > >>> mode for the original expression. The testcase would ICE
> > > >>> deap down while trying to do a move to use the target.
> > > >>> Anyways just calling expand_expr with NULL_EXPR fixes
> > > >>> the issue.
> > > >>>
> > > >>> Committed as obvious after a bootstrap/test on x86_64-linux-gnu.
> > > >>>
> > > >>>        PR middle-end/109919
> > > >>>
> > > >>> gcc/ChangeLog:
> > > >>>
> > > >>>        * expr.cc (expand_single_bit_test): Don't use the
> > > >>>        target for expand_expr.
> > > >>>
> > > >>> gcc/testsuite/ChangeLog:
> > > >>>
> > > >>>        * gcc.c-torture/compile/pr109919-1.c: New test.
> > > >> Thanks.  I'll respin the targets that failed.  If you don't hear from
> > > >> me, assume everything is happy again after this fix.
> > > >
> > > > Oh, I am going to test on aarch64-linux-gnu too just in case.
> > > > Expand is definitely something which I am not used to working on so I
> > > > figured I had made a mistake somewhere. I suspect I still made a
> > > > similar mistake later on too.
> > > I'm seeing some execution failures.  Building H8 bits now to debug as
> > > it's the target I'm most familiar with.   More info as it's available.
> >
> > Is H8 big-endian? I could have messed that up.
>
> If so, try the attached patch. I thought extract_bit_field's bitnum
> field was like a shift and not like how BIT_FIELD_REF is defined.

Forgot about poly modes requiring to use as_a <scalar_int_mode> to get
an integer mode's integer bitsize now.

So try this version (which now compiles).

Thanks,
Andrew

>
> Thanks,
> Andrew
>
> >
> > Thanks,
> > Andrew Pinski
> >
> > >
> > > jeff

[-- Attachment #2: maybefixbigendian.diff.txt --]
[-- Type: text/plain, Size: 641 bytes --]

diff --git a/gcc/expr.cc b/gcc/expr.cc
index 02f24c00148..050efcd0b00 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -12958,7 +12958,14 @@ expand_single_bit_test (location_t loc, enum tree_code code,
 
   rtx inner0 = expand_expr (inner, NULL_RTX, VOIDmode, EXPAND_NORMAL);
 
-  inner0 = extract_bit_field (inner0, 1, bitnum, 1, target,
+  int bitpos = bitnum;
+
+  scalar_int_mode imode = as_a <scalar_int_mode>(GET_MODE (inner0));
+
+  if (BYTES_BIG_ENDIAN)
+    bitpos = GET_MODE_BITSIZE (imode) - 1 - bitpos;
+
+  inner0 = extract_bit_field (inner0, 1, bitpos, 1, target,
 			      operand_mode, mode, 0, NULL);
 
   if (code == EQ_EXPR)

Re: [PATCH] Fix PR 109919: ICE in emit_move_insn with some bit tests

[Jeff Law]

On 5/20/23 21:28, Andrew Pinski wrote:
> On Sat, May 20, 2023 at 8:25 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>>
>> On 5/20/23 21:05, Andrew Pinski wrote:
>>> On Sat, May 20, 2023 at 6:26 PM Jeff Law via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>>
>>>>
>>>> On 5/20/23 19:09, Andrew Pinski via Gcc-patches wrote:
>>>>> The problem is I used expand_expr with the target but
>>>>> we don't want to use the target here as it is the wrong
>>>>> mode for the original expression. The testcase would ICE
>>>>> deap down while trying to do a move to use the target.
>>>>> Anyways just calling expand_expr with NULL_EXPR fixes
>>>>> the issue.
>>>>>
>>>>> Committed as obvious after a bootstrap/test on x86_64-linux-gnu.
>>>>>
>>>>>         PR middle-end/109919
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>>         * expr.cc (expand_single_bit_test): Don't use the
>>>>>         target for expand_expr.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>>         * gcc.c-torture/compile/pr109919-1.c: New test.
>>>> Thanks.  I'll respin the targets that failed.  If you don't hear from
>>>> me, assume everything is happy again after this fix.
>>>
>>> Oh, I am going to test on aarch64-linux-gnu too just in case.
>>> Expand is definitely something which I am not used to working on so I
>>> figured I had made a mistake somewhere. I suspect I still made a
>>> similar mistake later on too.
>> I'm seeing some execution failures.  Building H8 bits now to debug as
>> it's the target I'm most familiar with.   More info as it's available.
> 
> Is H8 big-endian? I could have messed that up.
It is.  In fact, that does seem to be common across the failing targets. 
  I'll respin them again with your latest fix.

jeff