| 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}) | expand |
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 >
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.
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.
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.