Re: [PATCH] Relax check against commuting XOR and ASHIFTRT in combine.c

[Rainer Orth <ro at CeBiTec dot Uni-Bielefeld dot DE>]

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
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c
@@ -1,4 +1,5 @@
-/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*-*-*} } */
+/* { dg-do compile { target aarch64*-*-* i?86-*-* 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
--- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
+++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c
@@ -1,4 +1,5 @@
-/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */
+/* { dg-do compile { target arm*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } } */
+/* { dg-require-effective-target ilp32 } */
 /* { dg-options "-O2 -fdump-rtl-combine-all" } */
 
 typedef long int32_t;

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University