Patchwork [middle-end] : Fix g++.dg/other/vector-compare.C testsuite failure on alpha

login
register
mail settings
Submitter Uros Bizjak
Date Sept. 18, 2012, 5:48 p.m.
Message ID <CAFULd4YJpCSHj8ec3iSco=Z=8viuSvtMbvfxrbx4f1hs1O+KiA@mail.gmail.com>
Download mbox | patch
Permalink /patch/184788/
State New
Headers show

Comments

Uros Bizjak - Sept. 18, 2012, 5:48 p.m.
Hello!

g++.dg/other/vector-compare.C recently started to fail on
alphaev68-pc-linux-gnu with:

[uros@localhost other]$ ~/gcc-build-alpha/gcc/cc1plus -std=gnu++11
-quiet vector-compare.C
vector-compare.C: In function ‘int main()’:
vector-compare.C:26:5: internal compiler error: in
emit_cmp_and_jump_insn_1, at optabs.c:4261
 int main ()
     ^
Please submit a full bug report,
with preprocessed source if appropriate.

Prepare_cmp_insn in optabs.c expands BLKmode compares using either
cmp{mem,str,strn}_optab, or through emit_library_call_value to integer
result register, and follows with the expansion of the compare of the
result with zero. However, the code blindly assumes that the target is
able to compare resulting SImode value, which is not true in case of
alpha. Due to missing SImode compare pattern, the above assert is
triggered in emit_cmp_and_jump_1.

The patch fixes this oversight by simply expanding the comparison of
the result through generic comparison expansion code that conveniently
follows BLKmode compare expansion.

2012-09-18  Uros Bizjak  <ubizjak@gmail.com>

	* optabs.c (prepare_cmp_insn): Expand comparison of the result
	of memory block compare through generic comparison expansion code.

Patch was bootstrapped and regression tested on alphaev68-pc-linux-gnu
and x86_64-pc-linux-gnu {,-m32}.

OK for mainline and release branches?

Thanks,
Uros.
Eric Botcazou - Sept. 21, 2012, 2:57 p.m.
> Prepare_cmp_insn in optabs.c expands BLKmode compares using either
> cmp{mem,str,strn}_optab, or through emit_library_call_value to integer
> result register, and follows with the expansion of the compare of the
> result with zero. However, the code blindly assumes that the target is
> able to compare resulting SImode value, which is not true in case of
> alpha. Due to missing SImode compare pattern, the above assert is
> triggered in emit_cmp_and_jump_1.
> 
> The patch fixes this oversight by simply expanding the comparison of
> the result through generic comparison expansion code that conveniently
> follows BLKmode compare expansion.
> 
> 2012-09-18  Uros Bizjak  <ubizjak@gmail.com>
> 
> 	* optabs.c (prepare_cmp_insn): Expand comparison of the result
> 	of memory block compare through generic comparison expansion code.

I don't think that the first hunk is necessary since result_mode is taken from 
the comparison pattern itself; the second hunk alone seems to be sufficient.

> Patch was bootstrapped and regression tested on alphaev68-pc-linux-gnu
> and x86_64-pc-linux-gnu {,-m32}.
> 
> OK for mainline and release branches?

Is that a regression on release branches?  If no, that's not worth the risk.
Uros Bizjak - Sept. 21, 2012, 3:11 p.m.
On Fri, Sep 21, 2012 at 4:57 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Prepare_cmp_insn in optabs.c expands BLKmode compares using either
>> cmp{mem,str,strn}_optab, or through emit_library_call_value to integer
>> result register, and follows with the expansion of the compare of the
>> result with zero. However, the code blindly assumes that the target is
>> able to compare resulting SImode value, which is not true in case of
>> alpha. Due to missing SImode compare pattern, the above assert is
>> triggered in emit_cmp_and_jump_1.
>>
>> The patch fixes this oversight by simply expanding the comparison of
>> the result through generic comparison expansion code that conveniently
>> follows BLKmode compare expansion.
>>
>> 2012-09-18  Uros Bizjak  <ubizjak@gmail.com>
>>
>>       * optabs.c (prepare_cmp_insn): Expand comparison of the result
>>       of memory block compare through generic comparison expansion code.
>
> I don't think that the first hunk is necessary since result_mode is taken from
> the comparison pattern itself; the second hunk alone seems to be sufficient.

Although the testcase doesn't trip on this part, the result_mode is
taken from the operand mode of cmp{mem,str,strn}_optab. As in the
second case, there is no guarantee that compare that compares the
result of cmp{mem,str,stn}_optab can handle output mode of this optab.

>> Patch was bootstrapped and regression tested on alphaev68-pc-linux-gnu
>> and x86_64-pc-linux-gnu {,-m32}.
>>
>> OK for mainline and release branches?
>
> Is that a regression on release branches?  If no, that's not worth the risk.

The testcase does not fail on release branches, but simply due to the
fact that BLKmode compares are not adequately tested. Otherwise,
BLKmode compares would always fail on alpha.

Uros.
Eric Botcazou - Sept. 21, 2012, 3:26 p.m.
> Although the testcase doesn't trip on this part, the result_mode is
> taken from the operand mode of cmp{mem,str,strn}_optab. As in the
> second case, there is no guarantee that compare that compares the
> result of cmp{mem,str,stn}_optab can handle output mode of this optab.

It's up to you to write a correct pattern with an appropriate result mode.

> The testcase does not fail on release branches, but simply due to the
> fact that BLKmode compares are not adequately tested. Otherwise,
> BLKmode compares would always fail on alpha.

What happens for the testcase on the release branches?  It doesn't use BKLmode 
compares?  If so, is it ever possible to use BLKmode compares on Alpha?
Uros Bizjak - Sept. 21, 2012, 3:42 p.m.
On Fri, Sep 21, 2012 at 5:26 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Although the testcase doesn't trip on this part, the result_mode is
>> taken from the operand mode of cmp{mem,str,strn}_optab. As in the
>> second case, there is no guarantee that compare that compares the
>> result of cmp{mem,str,stn}_optab can handle output mode of this optab.
>
> It's up to you to write a correct pattern with an appropriate result mode.

OK, I can live with this; if some other unfortunate target trips here,
we will know how to fix it.

>> The testcase does not fail on release branches, but simply due to the
>> fact that BLKmode compares are not adequately tested. Otherwise,
>> BLKmode compares would always fail on alpha.
>
> What happens for the testcase on the release branches?  It doesn't use BKLmode
> compares?  If so, is it ever possible to use BLKmode compares on Alpha?

Vector compare testcases can't be compiled with 4.7- - vector compares
are not implemented for C++ there. As observed, BLKmode compare is not
triggered for alpha in the whole testsuite, as far as release branches
are concerned.

Uros.
Eric Botcazou - Sept. 22, 2012, 10:54 a.m.
> Vector compare testcases can't be compiled with 4.7- - vector compares
> are not implemented for C++ there. As observed, BLKmode compare is not
> triggered for alpha in the whole testsuite, as far as release branches
> are concerned.

Then there is no real reason to touch the release branches in my opinion.

The second hunk is OK for mainline (without the now useless label).
Uros Bizjak - Sept. 22, 2012, 11:33 a.m.
On Sat, Sep 22, 2012 at 12:54 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Vector compare testcases can't be compiled with 4.7- - vector compares
>> are not implemented for C++ there. As observed, BLKmode compare is not
>> triggered for alpha in the whole testsuite, as far as release branches
>> are concerned.
>
> Then there is no real reason to touch the release branches in my opinion.

OK.

> The second hunk is OK for mainline (without the now useless label).

I will adjust the patch accordingly and commit it to mainline only.

Thanks for the review,
Uros.

Patch

Index: optabs.c
===================================================================
--- optabs.c	(revision 191413)
+++ optabs.c	(working copy)
@@ -4087,9 +4087,13 @@  prepare_cmp_insn (rtx x, rtx y, enum rtx_code comp
 	  size = convert_to_mode (cmp_mode, size, 1);
 	  emit_insn (GEN_FCN (cmp_code) (result, x, y, size, opalign));
 
-          *ptest = gen_rtx_fmt_ee (comparison, VOIDmode, result, const0_rtx);
-          *pmode = result_mode;
-	  return;
+	  x = result;
+	  y = const0_rtx;
+	  mode = result_mode;
+	  methods = OPTAB_LIB_WIDEN;
+	  unsignedp = false;
+
+	  goto result_compare;
 	}
 
       if (methods != OPTAB_LIB && methods != OPTAB_LIB_WIDEN)
@@ -4109,11 +4113,15 @@  prepare_cmp_insn (rtx x, rtx y, enum rtx_code comp
 					XEXP (y, 0), Pmode,
 					size, cmp_mode);
 
-      *ptest = gen_rtx_fmt_ee (comparison, VOIDmode, result, const0_rtx);
-      *pmode = result_mode;
-      return;
+      x = result;
+      y = const0_rtx;
+      mode = result_mode;
+      methods = OPTAB_LIB_WIDEN;
+      unsignedp = false;
     }
 
+ result_compare:
+
   /* Don't allow operands to the compare to trap, as that can put the
      compare and branch in different basic blocks.  */
   if (cfun->can_throw_non_call_exceptions)