BRIG FE testsuite: Fix all dump-scans (Was: Re: drop -aux{dir, base}, revamp -dump{dir, base})

Message ID ri6img0wgts.fsf@suse.cz
State New
Headers show
Series
  • BRIG FE testsuite: Fix all dump-scans (Was: Re: drop -aux{dir, base}, revamp -dump{dir, base})
Related show

Commit Message

Martin Jambor June 9, 2020, 12:42 p.m.
Hi,

On Tue, Jun 09 2020, Thomas Schwinge wrote:
> Hi!

>

> On 2020-05-26T04:08:44-0300, Alexandre Oliva <oliva@adacore.com> wrote:

>> Thanks, here's the combined patch I'm checking in.

>>

>> revamp dump and aux output names

>

> For BRIG (HSAIL) front end testing, I'm see a lot of failures like:

>

>     Running [...]/source-gcc/gcc/testsuite/brig.dg/dg.exp ...

>     PASS: brig.dg/test/gimple/variables.hsail (test for excess errors)

>     [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump original "__group_base_addr \\+ \\(0 \\+"

>     [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump original "__group_base_addr \\+ 0"

>     [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump gimple "[ ]*prog_global = s204;"

>     [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump gimple ".module.mod_global;"

>     [...]

>

> That's:

>

>     spawn -ignore SIGHUP [...]/build-gcc/gcc/xgcc -B[...]/build-gcc/gcc/ [...]/build-gcc/gcc/testsuite/brig/variables.hsail.brig -fdump-tree-gimple -fdump-tree-original -S -o variables.s

>     PASS: brig.dg/test/gimple/variables.hsail (test for excess errors)

>     variables.hsail.brig: dump file does not exist

>     dump file: variables.hsail.brig.original

>     UNRESOLVED: variables.hsail.brig scan-tree-dump original "__group_base_addr \\+ \\(0 \\+"

>

> We're trying to scan 'variables.hsail.brig.*', but for input file name

> 'variables.hsail.brig', we're now creating:

>

>     $ ls -1 build-gcc/gcc/testsuite/brig/variables.*???t.*

>     build-gcc/gcc/testsuite/brig/variables.brig.004t.original

>     build-gcc/gcc/testsuite/brig/variables.brig.005t.gimple

>

> Before your changes, GCC produced the expected:

>

>     $ ls -1 build-gcc/gcc/testsuite/brig/variables.*???t.*

>     build-gcc/gcc/testsuite/brig/variables.hsail.brig.004t.original

>     build-gcc/gcc/testsuite/brig/variables.hsail.brig.005t.gimple

>

> Are you able to easily create a patch for that?  How/where to adjust:

> producer-side (GCC driver, or BRIG (HSAIL) front end?), or consumer-side

> (testsuite: tree scanning machinery, or have to put some '-dumpbase' into

> all test case files?)?



I looked into the issue yesterday and decided the simplest fix is
probably the following.  I am going to use my BRIG maintainer hat to
commit the patch in a day or two unless someone thinks it is a bad idea.
Tested by running make check-brig on an x86_64-linux.

Martin



Since Alexandre's revamp of dump files handling in
r11-627-g1dedc12d186, BRIG FE has been receiving slightly different
-dumpbase (e.g. smoke_test.brig instead of smoke_test.hsail.brig when
compiling file smoke_test.hsail.brig) and the testsuite then could not
find the generated dump files it wanted to scan.  I have not really
looked into why that changed, the easiest fix seems to me to remove
the hsail part already when generating the binary brig file from the
textual HSAIL representation.

gcc/testsuite/ChangeLog:

2020-06-09  Martin Jambor  <mjambor@suse.cz>

	* lib/brig.exp (brig_target_compile): Strip hsail extension when
	gnerating the name of the binary brig file.
---
 gcc/testsuite/lib/brig.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.26.2

Comments

I think I'd prefer the fix on the other side, if reasonable.  I'd give them some time to see about a fix there before selecting this patch.

On Jun 9, 2020, at 5:42 AM, Martin Jambor <mjambor@suse.cz> wrote:
> On Tue, Jun 09 2020, Thomas Schwinge wrote:

>> On 2020-05-26T04:08:44-0300, Alexandre Oliva <oliva@adacore.com> wrote:

>>> Thanks, here's the combined patch I'm checking in.

>>> 

>>> revamp dump and aux output names

>> 

>> For BRIG (HSAIL) front end testing, I'm see a lot of failures like:

>> 

>>    Running [...]/source-gcc/gcc/testsuite/brig.dg/dg.exp ...

>>    PASS: brig.dg/test/gimple/variables.hsail (test for excess errors)

>>    [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump original "__group_base_addr \\+ \\(0 \\+"

>>    [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump original "__group_base_addr \\+ 0"

>>    [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump gimple "[ ]*prog_global = s204;"

>>    [-PASS:-]{+UNRESOLVED:+} variables.hsail.brig scan-tree-dump gimple ".module.mod_global;"

>>    [...]

>> 

>> That's:

>> 

>>    spawn -ignore SIGHUP [...]/build-gcc/gcc/xgcc -B[...]/build-gcc/gcc/ [...]/build-gcc/gcc/testsuite/brig/variables.hsail.brig -fdump-tree-gimple -fdump-tree-original -S -o variables.s

>>    PASS: brig.dg/test/gimple/variables.hsail (test for excess errors)

>>    variables.hsail.brig: dump file does not exist

>>    dump file: variables.hsail.brig.original

>>    UNRESOLVED: variables.hsail.brig scan-tree-dump original "__group_base_addr \\+ \\(0 \\+"

>> 

>> We're trying to scan 'variables.hsail.brig.*', but for input file name

>> 'variables.hsail.brig', we're now creating:

>> 

>>    $ ls -1 build-gcc/gcc/testsuite/brig/variables.*???t.*

>>    build-gcc/gcc/testsuite/brig/variables.brig.004t.original

>>    build-gcc/gcc/testsuite/brig/variables.brig.005t.gimple

>> 

>> Before your changes, GCC produced the expected:

>> 

>>    $ ls -1 build-gcc/gcc/testsuite/brig/variables.*???t.*

>>    build-gcc/gcc/testsuite/brig/variables.hsail.brig.004t.original

>>    build-gcc/gcc/testsuite/brig/variables.hsail.brig.005t.gimple

>> 

>> Are you able to easily create a patch for that?  How/where to adjust:

>> producer-side (GCC driver, or BRIG (HSAIL) front end?), or consumer-side

>> (testsuite: tree scanning machinery, or have to put some '-dumpbase' into

>> all test case files?)?

> 

> 

> I looked into the issue yesterday and decided the simplest fix is

> probably the following.  I am going to use my BRIG maintainer hat to

> commit the patch in a day or two unless someone thinks it is a bad idea.

> Tested by running make check-brig on an x86_64-linux.

> 

> Martin

> 

> 

> 

> Since Alexandre's revamp of dump files handling in

> r11-627-g1dedc12d186, BRIG FE has been receiving slightly different

> -dumpbase (e.g. smoke_test.brig instead of smoke_test.hsail.brig when

> compiling file smoke_test.hsail.brig) and the testsuite then could not

> find the generated dump files it wanted to scan.  I have not really

> looked into why that changed, the easiest fix seems to me to remove

> the hsail part already when generating the binary brig file from the

> textual HSAIL representation.

> 

> gcc/testsuite/ChangeLog:

> 

> 2020-06-09  Martin Jambor  <mjambor@suse.cz>

> 

> 	* lib/brig.exp (brig_target_compile): Strip hsail extension when

> 	gnerating the name of the binary brig file.

> ---

> gcc/testsuite/lib/brig.exp | 2 +-

> 1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/gcc/testsuite/lib/brig.exp b/gcc/testsuite/lib/brig.exp

> index fbfb1da947a..de47f13e42c 100644

> --- a/gcc/testsuite/lib/brig.exp

> +++ b/gcc/testsuite/lib/brig.exp

> @@ -29,7 +29,7 @@ proc brig_target_compile { source dest type options } {

> 	# We cannot assume all inputs are .hsail as the dg machinery

> 	# calls this for a some c files to check linker plugin support or

> 	# similar.

> -	set brig_source ${tmpdir}/[file tail ${source}].brig

> +	set brig_source ${tmpdir}/[file rootname [file tail ${source}]].brig

> 	exec HSAILasm $source -o ${brig_source}

> 	set source ${brig_source}

> 	# Change the testname the .brig.

> -- 

> 2.26.2

>
Alexandre Oliva June 10, 2020, 9:50 p.m. | #2
On Jun  9, 2020, Martin Jambor <mjambor@suse.cz> wrote:

>> Before your changes, GCC produced the expected:

>> 

>> $ ls -1 build-gcc/gcc/testsuite/brig/variables.*???t.*

>> build-gcc/gcc/testsuite/brig/variables.hsail.brig.004t.original

>> build-gcc/gcc/testsuite/brig/variables.hsail.brig.005t.gimple

>> 

>> Are you able to easily create a patch for that?  How/where to adjust:

>> producer-side (GCC driver, or BRIG (HSAIL) front end?), or consumer-side

>> (testsuite: tree scanning machinery, or have to put some '-dumpbase' into

>> all test case files?)?



> I looked into the issue yesterday and decided the simplest fix is

> probably the following.


I concur, FWIW.  Thanks for looking into it.

Since dumpbase is now based on dest rather than src, at least for
compilation without linking, a change that aligns source and dest
basenames makes for a least-surprise scenario.

I suppose we might want to eventually adjust the expectations of dump
names in the testsuite to compute names based on outputs rather than
inputs, or based on both when compiling and linking, but that will
require quite significant internal API changes in the testsuite
infrastructure.  I'm sort of hoping we can make do with what we got, at
least until the new naming conventions are solidly established.

> 	* lib/brig.exp (brig_target_compile): Strip hsail extension when

> 	gnerating the name of the binary brig file.


LGTM, thanks.

-- 
Alexandre Oliva, freedom fighter    he/him    https://FSFLA.org/blogs/lxo/
Free Software Evangelist              Stallman was right, but he's left :(
GNU Toolchain Engineer           Live long and free, and prosper ethically
Martin Jambor June 11, 2020, 2:28 p.m. | #3
Hi Mike,

On Tue, Jun 09 2020, Mike Stump wrote:
> I think I'd prefer the fix on the other side, if reasonable.  I'd give

> them some time to see about a fix there before selecting this patch.


given Alexandre's email, are you OK with the patch?  It essentially
manually keeps the input name "rootname" in sync with the output file
"rootname" which is the new basis for dump names.  As Alexandre noted,
the proper fix is probably to teach the testsuite to expect dump names
based on outputs by default but I do not want to leave the majority of
BRIG testcases broken until that happens.

Thanks,

Martin


>

> On Jun 9, 2020, at 5:42 AM, Martin Jambor <mjambor@suse.cz> wrote:


[...]

>> 

>> 

>> I looked into the issue yesterday and decided the simplest fix is

>> probably the following.  I am going to use my BRIG maintainer hat to

>> commit the patch in a day or two unless someone thinks it is a bad idea.

>> Tested by running make check-brig on an x86_64-linux.

>> 

>> Martin

>> 

>> 

>> 

>> Since Alexandre's revamp of dump files handling in

>> r11-627-g1dedc12d186, BRIG FE has been receiving slightly different

>> -dumpbase (e.g. smoke_test.brig instead of smoke_test.hsail.brig when

>> compiling file smoke_test.hsail.brig) and the testsuite then could not

>> find the generated dump files it wanted to scan.  I have not really

>> looked into why that changed, the easiest fix seems to me to remove

>> the hsail part already when generating the binary brig file from the

>> textual HSAIL representation.

>> 

>> gcc/testsuite/ChangeLog:

>> 

>> 2020-06-09  Martin Jambor  <mjambor@suse.cz>

>> 

>> 	* lib/brig.exp (brig_target_compile): Strip hsail extension when

>> 	gnerating the name of the binary brig file.

>> ---

>> gcc/testsuite/lib/brig.exp | 2 +-

>> 1 file changed, 1 insertion(+), 1 deletion(-)

>> 

>> diff --git a/gcc/testsuite/lib/brig.exp b/gcc/testsuite/lib/brig.exp

>> index fbfb1da947a..de47f13e42c 100644

>> --- a/gcc/testsuite/lib/brig.exp

>> +++ b/gcc/testsuite/lib/brig.exp

>> @@ -29,7 +29,7 @@ proc brig_target_compile { source dest type options } {

>> 	# We cannot assume all inputs are .hsail as the dg machinery

>> 	# calls this for a some c files to check linker plugin support or

>> 	# similar.

>> -	set brig_source ${tmpdir}/[file tail ${source}].brig

>> +	set brig_source ${tmpdir}/[file rootname [file tail ${source}]].brig

>> 	exec HSAILasm $source -o ${brig_source}

>> 	set source ${brig_source}

>> 	# Change the testname the .brig.

>> -- 

>> 2.26.2

>>
On Jun 11, 2020, at 7:28 AM, Martin Jambor <mjambor@suse.cz> wrote:
> 

> On Tue, Jun 09 2020, Mike Stump wrote:

>> I think I'd prefer the fix on the other side, if reasonable.  I'd give

>> them some time to see about a fix there before selecting this patch.

> 

> given Alexandre's email, are you OK with the patch?


Yeah, if that's the change dumpbase is going, then we can follow along with it.  Ok.

Patch

diff --git a/gcc/testsuite/lib/brig.exp b/gcc/testsuite/lib/brig.exp
index fbfb1da947a..de47f13e42c 100644
--- a/gcc/testsuite/lib/brig.exp
+++ b/gcc/testsuite/lib/brig.exp
@@ -29,7 +29,7 @@  proc brig_target_compile { source dest type options } {
 	# We cannot assume all inputs are .hsail as the dg machinery
 	# calls this for a some c files to check linker plugin support or
 	# similar.
-	set brig_source ${tmpdir}/[file tail ${source}].brig
+	set brig_source ${tmpdir}/[file rootname [file tail ${source}]].brig
 	exec HSAILasm $source -o ${brig_source}
 	set source ${brig_source}
 	# Change the testname the .brig.