diff mbox series

Fix PR 109919: ICE in emit_move_insn with some bit tests

Message ID 20230521010949.1957550-1-apinski@marvell.com
State Unresolved
Headers
Series Fix PR 109919: ICE in emit_move_insn with some bit tests |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Andrew Pinski May 21, 2023, 1:09 a.m. UTC 
  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

Comments

Jeff Law May 21, 2023, 1:25 a.m. UTC | #1 
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
Andrew Pinski May 21, 2023, 3:05 a.m. UTC | #2 
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
Jeff Law May 21, 2023, 3:25 a.m. UTC | #3 
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
Andrew Pinski May 21, 2023, 3:28 a.m. UTC | #4 
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
Andrew Pinski May 21, 2023, 3:32 a.m. UTC | #5 
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
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)
Andrew Pinski May 21, 2023, 3:47 a.m. UTC | #6 
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
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)
Jeff Law May 21, 2023, 4:34 a.m. UTC | #7 
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
diff mbox series

Patch

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;
+}