diff mbox

Relax check against commuting XOR and ASHIFTRT in combine.c

Message ID yddvbjpiswz.fsf@lokon.CeBiTec.Uni-Bielefeld.DE
State New
Headers show

Commit Message

Rainer Orth Jan. 29, 2015, 2:05 p.m. UTC
Alan,

sorry for letting the ball drop on this one.

> Well, I'd be happy with that. Curiously, that patch is identical to what I
> have here...and that's not what I got having built gcc with
> --target=sparc-sun-solaris2.11, but on further investigation it looks like
> the require-effective-target-ilp32/64 is not working correctly on my setup.
>
> FWIW I've also tried MIPS (both 32- and 64-bit tests appear to work), IA64
> (64-bit test fine, not sure if ilp32 is supported), microblaze (32-bit),

No, there's no 32-bit IA-64.

> and AArch64 with -mabi=ilp32. Hence, I attach an alternative/possible patch
> which adds these platforms too.

Huh?  The two patches seem completely identical to me...

> However, since we have talked about removing the target selector
> altogether: the only arch I've found so far which has failed, was Alpha,
> and widening the regex to match "le" as well as "ge" then passes on Alpha
> too. To be honest I expect that if we were to go that route, we would find
> a deluge of smaller platforms on which the test fails; but if as testsuite
> maintainer you think that's appropriate - certainly I'd be willing to try
> that i.e. to exclude them as they turn up. A second alternative patch also
> attached. (FWIW I'll be away and unable to commit anything before Monday.)

I'm still not really comfortable with those target lists; they tend to
artificially exclude tests on targets where they are perfectly capable
of running.  At least with the comments added, it's better than before
with no explanation whatsoever.  Perhaps Mike can weigh in here?

> More generally: really the test wants to be a unit test on
> combine_simplify_rtx, independent of front-end, expand, platform-specific
> insns, etc., but since we can't do that - whilst adding more platforms
> generally seems good, it is maybe not essential, and may increase
> fragility.

Might be, though we'll never know until we try.

> (In answer to your point about adding an effective-target in
> target-supports.exp - yes, could do that, but it's difficult to come up
> with a good characterization of what the criteria is, and I don't see it'd
> generalize to any other tests at all :(....)

Agreed: if this list isn't going to be used beyond those two tests,
there's no point in adding some artifical effective-target keyword.

I'm attaching an updated version of the patch with a few minor changes
of mine, noted below.

	Rainer


> Rainer Orth wrote:
>> Alan Lawrence <alan.lawrence@arm.com> writes:
>>
>>> Rainer Orth wrote:
>>>>> However, as a quick first step, does adding the ilp32 / lp64 (and keeping
>>>>> the architectures list for now) solve the immediate problem? Patch
>>>>> attached, OK for trunk?
>>>> No, as I said this is wrong for biarch targets like sparc and i386.
>>> When you say no this does not solve the immediate problem, are you saying
>>> that you are (still) seeing test failures with the require-effective-target
>>> patch applied? Or is the issue that this would not execute the tests as
>>
>> I didn't try that patch yet, but the target part is wrong, as I tried to
>> explain.  Consider the sparc case: 
>>
>> * if you configure for sparc-sun-solaris2.11, you default to -m32
>>   (i.e. ilp32), while -m64 is lp64
>>
>> * if you configure for sparcv9-sun-solaris2.11 instead, you default to
>>   -m64 (lp64), but get ilp32 with -m32
>>
>> So, irrespective of the sparc vs. sparc64 (which is wrong, btw., the
>> canonical form for 64-bit-default sparc is sparcv9) forms, you can get
>> ilp32 and lp64 with both.
>>
>> Similar issues hold for i?86 vs. x86_64 and probably other biarch
>> targets like powerpc vs. powerpc64, so you need to use the most generic
>> forms of the target names in you target lists.
>>
>>> widely as might be possible? In principle I'm quite happy to relax the
>>> target patterns, although have been having issues with sparc (below)...
>>>
>>> Re. "what the architectures have in common" is largely that these are the
>>> primary/secondary archs on which I've checked the test passes! I can now
>>> add mips and microblaze to this list, however I'm nervous of dropping the
>>> target entirely given the very large number of target architectures gcc
>>> supports; and e.g. IA64 (in ILP32 mode) generates an ashiftrt:DI by 31
>>> places, not ashiftrt:SI, which does not match the simplification criteria
>>> in combine.c.
>>
>> As I stated before, such target lists without any explanation are bound
>> to confuse future readers/testers: at the very least, add comments
>> explaining what those lists have in common.  OTOH, at this stage it
>> might be best to just drop the target list for now, learn which targets
>> pass and fail the tests, and then reintroduce them or, better yet, add
>> an effective-target keyword which matches them.  Otherwise, you'll never
>> get test coverage beyond your current list.
>>
>>>> This should be something like 
>>>>
>>>>   { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* }
>>>>
>>>> E.g. sparc-sun-solaris2.11 with -m64 is lp64, but would be excluded by
>>>> your target list.  Keep the list sorted alphabetically and best add an
>>>> explanation so others know what those targets have in common.
>>> So I've built a stage-1 compiler with --target=sparc-sun-solaris2.11,
>>> and I find
>>>
>>>   * without -m64, my "dg-require-effective-target ilp32" causes the 32-bit
>>> test to execute, and pass; "dg-require-effective-target lp64" prevents
>>> execution of the 64-bit test (which would fail) - so all as expected and
>>> desired.
>>>
>>>   * with -lp64, behaviour is as previous (this is probably expected)
>>
>> Huh?  What's -lp64?
>>
>>>   * with -m64, "dg-require-effective-target ilp32" still causes the test to
>>> execute (but it fails, as the RTL now has an ashiftrt:DI by 31 places,
>>> which doesn't meet the simplification criteria in combine.c - this is
>>> pretty much as expected). "dg-require-effective-target lp64" stops the
>>> 64-bit test from executing however (despite that it would now pass).
>>>
>>> Can you clarify what I should be doing on sparc, therefore?
>>
>> It's not only about sparc, but about all biarch targets.  The following
>> patch (which only includes the parts strictly necessary to avoid the
>> failures, nothing else I suggested above) works for me on
>> sparc-sun-solaris2.11 (-m32 and -m64), x86_64-unknown-linux-gnu (-m64
>> and -m32), and i686-unknown-linux-gnu (-m32 and -m64): the first test is
>> run for 64-bit only, while the second one only for 32-bit:
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f943030b77583bb7570 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
> +/* Target architectures which have been found to produce the expected RTL
> +   (neg:DI (ge:DI ...)) when compiling for ILP32.  */

LP64 here.

> +/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */

Please have whitespace here: "{ target" and here: "x86_64-*-* }".

> +/* { dg-require-effective-target lp64 } */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long long int int64_t;
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c15d6b6c74930d 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
> +/* Target architectures where RTL has been found to produce the expected
> +   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
> +/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */

Same whitespace issue...

> +/* { dg-require-effective-target ilp32} */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long int32_t;
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f943030b77583bb7570 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
> +/* Target architectures which have been found to produce the expected RTL
> +   (neg:DI (ge:DI ...)) when compiling for ILP32.  */
> +/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
> +/* { dg-require-effective-target lp64 } */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long long int int64_t;
> diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c15d6b6c74930d 100644
> --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
> @@ -1,4 +1,7 @@
> -/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
> +/* Target architectures where RTL has been found to produce the expected
> +   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
> +/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */
> +/* { dg-require-effective-target ilp32} */
>  /* { dg-options "-O2 -fdump-rtl-combine-all" } */
>  
>  typedef long int32_t;


2015-01-29  Alan Lawrence  <alan.lawrence@arm.com>
	    Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	gcc/testsuite:
	* gcc.dg/combine_ashiftrt_1.c: Sort, complete and explain target
	list, allow for multilibed targets.
	* gcc.dg/combine_ashiftrt_2.c: Likewise.

Comments

Alan Lawrence Feb. 2, 2015, 2:33 p.m. UTC | #1
Rainer Orth wrote:
>
> I'm still not really comfortable with those target lists; they tend to
> artificially exclude tests on targets where they are perfectly capable
> of running.  At least with the comments added, it's better than before
> with no explanation whatsoever.  Perhaps Mike can weigh in here?

Well, it's been awhile, but on further reflection - my feeling is that we should 
be dropping the target lists here too. Maybe we end up introducing a dg-skip-if 
that grows over time, but it'd have to grow quite a bit to reach the size of the 
dg-do target we'd otherwise have...

However I am a bit wary about dropping the dg-do target constraint just as we 
are nearing a release! So if we were to keep the whitelist approach, your patch 
looks good to me, and I'd be happy if that were committed.

Cheers, Alan
Rainer Orth Feb. 2, 2015, 3:47 p.m. UTC | #2
Hi Alan,

>> I'm still not really comfortable with those target lists; they tend to
>> artificially exclude tests on targets where they are perfectly capable
>> of running.  At least with the comments added, it's better than before
>> with no explanation whatsoever.  Perhaps Mike can weigh in here?
>
> Well, it's been awhile, but on further reflection - my feeling is that we
> should be dropping the target lists here too. Maybe we end up introducing a
> dg-skip-if that grows over time, but it'd have to grow quite a bit to reach
> the size of the dg-do target we'd otherwise have...

It's not even necessary to use dg-skip if the scan-rtl-dump fails.  You
can just add an xfail there, which has the advantage that you do notice
if the test starts to pass e.g. due to changes in a target.

> However I am a bit wary about dropping the dg-do target constraint just as
> we are nearing a release! So if we were to keep the whitelist approach,
> your patch looks good to me, and I'd be happy if that were committed.

Let's give others a day or two to comment: if nobody is in favour of the
more agressive approach, I'll commit my patch.

Thanks.
        Rainer
diff mbox

Patch

# HG changeset patch
# Parent aedb8ec64c1c4c74e30210d024845e2b0b2dc1eb
Properly check for 32 vs. 64-bit in gcc.dg/combine_ashiftrt_[12].c

diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -1,4 +1,6 @@ 
-/* { dg-do compile {target sparc64*-*-* aarch64*-*-* i?86-*-* x86_64-*-* powerpc64*-*-*} } */
+/* Target architectures which have been found to produce the expected RTL
+   (neg:DI (ge:DI ...)) when compiling for LP64.  */
+/* { dg-do compile { target aarch64*-*-* arm*-*-* i?86-*-* ia64-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } } */
 /* { dg-require-effective-target lp64 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -1,4 +1,6 @@ 
-/* { dg-do compile {target arm*-*-* i?86-*-* x86_64-*-* powerpc-*-* sparc-*-*} } */
+/* Target architectures where RTL has been found to produce the expected
+   (neg:SI (ge:SI ...)) when compiling for ILP32.  */
+/* { dg-do compile { target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* mips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } } */
 /* { dg-require-effective-target ilp32 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */