Kewen, Peter, Segher:
On 6/17/24 19:56, Kewen.Lin wrote:
> Hi,
>
> on 2024/6/18 00:08, Peter Bergner wrote:
>> On 6/14/24 1:37 PM, Carl Love wrote:
>>> Per the additional feedback after patch:
>>>
>>> commit c892525813c94b018464d5a4edc17f79186606b7
>>> Author: Carl Love <c...@linux.ibm.com>
>>> Date: Tue Jun 11 14:01:16 2024 -0400
>>>
>>> rs6000, altivec-2-runnable.c should be a runnable test
>>>
>>> The test case has "dg-do compile" set not "dg-do run" for a runnable
>>> test. This patch changes the dg-do command argument to run.
>>>
>>> gcc/testsuite/ChangeLog:gcc/testsuite/ChangeLog:
>>> * gcc.target/powerpc/altivec-2-runnable.c: Change dg-do
>>> argument to run.
>>
>> Test case altivec-1-runnable.c seems to have the same issue, in that it
>> is currently a dg-do compile test case rather than the intended dg-do run.
>
> Good catch!
OK, will update that as well. I think it will need the same header as
altivec-2-runnable.c
so once we have a final change for altivec-2-runnable.c, I will make the header
for
altivec-1-runnable.c be the same.
>
>> Can you have a look at changing that to dg-do run too? My guess it that
>> this one will want something similar to some other altivec test cases, ala:
>>
>> /* { dg-do run { target vmx_hw } } */
>> /* { dg-do compile { target { ! vmx_hw } } } */
>> /* { dg-require-effective-target powerpc_altivec_ok } */
>> /* { dg-options "-O2 -maltivec -mabi=altivec" } */
>
> I'd expect the "-runnable" test case focuses on testing for run. Normally,
> the one without "-runnable" would focus on testing for compiling (scan some
> desired insn), but this altivec-1.c and altivec-1-runnable.c seems to test
> for different things, maybe we should separate them into different names
> if they don't test for a same test point.
The altivec-1-runnable.c and altivec-2-runnable.c tests were added for various
built-ins that didn't have any test cases. There wasn't an intention that
there was
any connection to the existing altivec-*.c test files. I started creating
runnable
when I started adding support for built-ins that we claimed to support but had
never
actually been implemented. I created runnable tests to make sure my
implementation
actually worked. I continued to add runnable tests for built-ins
that existed but didn't have a test case. Adding runnable tests did find a
couple
of issues where the existing implementation had a bug.
That all said, if we want tochange the name of altivec-1-runnable.c and
altivec-2-runnable.c a different naming scheme that is fine with me. Perhaps we
should
finish fixing the header for this test file, then do altivec-1-runnable, and
then
a final patch that does all the file renaming?
>
>>
>> That said, I don't like not having a -mdejagnu-cpu=... here.
>> I think for our server cpus, this is fine, but on an embedded system
>> with a old ISA default for -mcpu=... (so we be doing a dg-do compile),
>> just adding -maltivec to that default may not make much sense for that
>> default and probably should be an error. Maybe something like:
>
> Yes, for some embedded cpus, there will be some error messages, but since
> we have powerpc_altivec_ok effective target, the error would make that
> effective target checking fail so I'd expect it'll stop it being tested
> (unsupported).
>
>>
>> /* { dg-do run { target vmx_hw } } */
>> /* { dg-do compile { target { ! vmx_hw } } } */
>> /* { dg-require-effective-target powerpc_altivec_ok } */
>> /* { dg-options "-O2 -mdejagnu=power7" } */
>>
>> ...makes more sense? Ke Wen & Segher, thoughts on that?
>> Ke Wen, should powerpc_altivec_ok be powerpc_altivec here???
>
> Yes, I just pushed r15-1390 for this change.
>
> BR,
> Kewen
>
We had -mdejagnu=power8 before, but it looks like we want to go to power7 now.
It sounds like we want the following:
/* { dg-do run { target vmx_hw } } */
/* { dg-do compile { target { ! vmx_hw } } } */
/* { dg-options "-O2 -mdejagnu=power7" } */
/* { dg-require-effective-target powerpc_altivec } */
Carl
