| Message ID | 20170921144831.GA24114@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [TESTSUITE,ARM,COMMITTED] Invert check to misalign in vect_hw_misalign (PR 78421) | expand |
On 21 September 2017 at 16:48, Tamar Christina <tamar.christina@arm.com> wrote: > Hi All, > > Commit r244796 changed vect_hw_misalign for arm to check against > arm_vect_no_misalign. However vect_hw_misalign is supposed to check if > a target supports misalign access, while arm_vect_no_misalign checks that > a target only supports aligned access. > > As such the results need to be inverted otherwise the test runs in exactly the > wrong circumstances. > > Committed as r253073 under the GCC obvious rule. Hi Tamar, This is not as obvious as we might think. This patch is causing tcl syntax errors, such as: error executing dg-final: expected boolean value but got "!1" I plan to send a fix b/o next week. Christophe > > Thanks, > Tamar > > gcc/ChangeLog > 2017-09-21 Tamar Christina <tamar.christina@arm.com> > > PR testsuite/78421 > * lib/target-supports.exp (check_effective_target_vect_hw_misalign): > Invert arm check. > > --
On 23 September 2017 at 16:12, Christophe Lyon <christophe.lyon@linaro.org> wrote: > On 21 September 2017 at 16:48, Tamar Christina <tamar.christina@arm.com> wrote: >> Hi All, >> >> Commit r244796 changed vect_hw_misalign for arm to check against >> arm_vect_no_misalign. However vect_hw_misalign is supposed to check if >> a target supports misalign access, while arm_vect_no_misalign checks that >> a target only supports aligned access. >> >> As such the results need to be inverted otherwise the test runs in exactly the >> wrong circumstances. >> >> Committed as r253073 under the GCC obvious rule. > > Hi Tamar, > > This is not as obvious as we might think. This patch is causing tcl > syntax errors, > such as: > error executing dg-final: expected boolean value but got "!1" > > I plan to send a fix b/o next week. > The attached patch would apply after reverting yours. I've applied it against r253072 (just before your patch) and the results are visible at: http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/253072-hw-misalign2/report-build-info.html Do they match your expectations? It looks like a few testcases need to be adjusted, or new bugs are uncovered. If the patch OK with a suitable ChangeLog entry? Thanks, Christophe > Christophe > >> >> Thanks, >> Tamar >> >> gcc/ChangeLog >> 2017-09-21 Tamar Christina <tamar.christina@arm.com> >> >> PR testsuite/78421 >> * lib/target-supports.exp (check_effective_target_vect_hw_misalign): >> Invert arm check. >> >> -- diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 2e0e4d43..133b1ac 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -5951,7 +5951,14 @@ proc check_effective_target_vect_hw_misalign { } { set et_vect_hw_misalign_saved($et_index) 1 } if { [istarget arm*-*-*] } { - set et_vect_hw_misalign_saved($et_index) [check_effective_target_arm_vect_no_misalign] + # We just want to invert the (boolean) value of + # arm_vect_no_misalign, but using the ! operator does not + # work when callers use the result of vect_hw_misalign + if { [check_effective_target_arm_vect_no_misalign] } { + set et_vect_hw_misalign_saved($et_index) 0 + } else { + set et_vect_hw_misalign_saved($et_index) 1 + } } } verbose "check_effective_target_vect_hw_misalign:\
> -----Original Message----- > From: Christophe Lyon [mailto:christophe.lyon@linaro.org] > Sent: 23 September 2017 18:52 > To: Tamar Christina > Cc: gcc-patches@gcc.gnu.org; nd; Ramana Radhakrishnan; Richard Earnshaw; > nickc@redhat.com; Kyrylo Tkachov > Subject: Re: [GCC][PATCH][TESTSUITE][ARM][COMMITTED] Invert check to > misalign in vect_hw_misalign (PR 78421) > > On 23 September 2017 at 16:12, Christophe Lyon > <christophe.lyon@linaro.org> wrote: > > On 21 September 2017 at 16:48, Tamar Christina > <tamar.christina@arm.com> wrote: > >> Hi All, > >> > >> Commit r244796 changed vect_hw_misalign for arm to check against > >> arm_vect_no_misalign. However vect_hw_misalign is supposed to check > >> if a target supports misalign access, while arm_vect_no_misalign > >> checks that a target only supports aligned access. > >> > >> As such the results need to be inverted otherwise the test runs in > >> exactly the wrong circumstances. > >> > >> Committed as r253073 under the GCC obvious rule. > > > > Hi Tamar, > > Hi Christoph, > > This is not as obvious as we might think. This patch is causing tcl > > syntax errors, such as: > > error executing dg-final: expected boolean value but got "!1" > > Ack, I didn't see this before, I only checked for tests using arm_vect_no_misalign and retested those and the few That were failing before. Sorry about that. > > I plan to send a fix b/o next week. > > > > The attached patch would apply after reverting yours. > I've applied it against r253072 (just before your patch) and the results are > visible at: > http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test- > patches/253072-hw-misalign2/report-build-info.html > > Do they match your expectations? It looks like a few testcases need to be > adjusted, or new bugs are uncovered. > > If the patch OK with a suitable ChangeLog entry? That looks OK to me, about ~40 new tests correctly run now, and I think the new failures are just test-isms but I'll Look at them more closely today. Thanks, Tamar > > Thanks, > > Christophe > > > Christophe > > > >> > >> Thanks, > >> Tamar > >> > >> gcc/ChangeLog > >> 2017-09-21 Tamar Christina <tamar.christina@arm.com> > >> > >> PR testsuite/78421 > >> * lib/target-supports.exp > (check_effective_target_vect_hw_misalign): > >> Invert arm check. > >> > >> --
On 25 September 2017 at 11:36, Tamar Christina <Tamar.Christina@arm.com> wrote: > > >> -----Original Message----- >> From: Christophe Lyon [mailto:christophe.lyon@linaro.org] >> Sent: 23 September 2017 18:52 >> To: Tamar Christina >> Cc: gcc-patches@gcc.gnu.org; nd; Ramana Radhakrishnan; Richard Earnshaw; >> nickc@redhat.com; Kyrylo Tkachov >> Subject: Re: [GCC][PATCH][TESTSUITE][ARM][COMMITTED] Invert check to >> misalign in vect_hw_misalign (PR 78421) >> >> On 23 September 2017 at 16:12, Christophe Lyon >> <christophe.lyon@linaro.org> wrote: >> > On 21 September 2017 at 16:48, Tamar Christina >> <tamar.christina@arm.com> wrote: >> >> Hi All, >> >> >> >> Commit r244796 changed vect_hw_misalign for arm to check against >> >> arm_vect_no_misalign. However vect_hw_misalign is supposed to check >> >> if a target supports misalign access, while arm_vect_no_misalign >> >> checks that a target only supports aligned access. >> >> >> >> As such the results need to be inverted otherwise the test runs in >> >> exactly the wrong circumstances. >> >> >> >> Committed as r253073 under the GCC obvious rule. >> > >> > Hi Tamar, >> > > > Hi Christoph, > >> > This is not as obvious as we might think. This patch is causing tcl >> > syntax errors, such as: >> > error executing dg-final: expected boolean value but got "!1" >> > > > Ack, I didn't see this before, I only checked for tests using arm_vect_no_misalign and retested those and the few > That were failing before. Sorry about that. > The tcl error messages may be difficult to notice, they tend to be "hidden" in the middle of the logs. >> > I plan to send a fix b/o next week. >> > >> >> The attached patch would apply after reverting yours. >> I've applied it against r253072 (just before your patch) and the results are >> visible at: >> http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test- >> patches/253072-hw-misalign2/report-build-info.html >> >> Do they match your expectations? It looks like a few testcases need to be >> adjusted, or new bugs are uncovered. >> >> If the patch OK with a suitable ChangeLog entry? > > That looks OK to me, about ~40 new tests correctly run now, and I think the new failures are just test-isms but I'll > Look at them more closely today. > Thanks for taking a look. I've added Mike in cc so that he can confirm my patch is right. There might be a better tcl idiom.... Thanks, Christophe > Thanks, > Tamar > >> >> Thanks, >> >> Christophe >> >> > Christophe >> > >> >> >> >> Thanks, >> >> Tamar >> >> >> >> gcc/ChangeLog >> >> 2017-09-21 Tamar Christina <tamar.christina@arm.com> >> >> >> >> PR testsuite/78421 >> >> * lib/target-supports.exp >> (check_effective_target_vect_hw_misalign): >> >> Invert arm check. >> >> >> >> --
On Sep 23, 2017, at 10:52 AM, Christophe Lyon <christophe.lyon@linaro.org> wrote: > The attached patch would apply after reverting yours. > I've applied it against r253072 (just before your patch) and the > results are visible at: > http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/253072-hw-misalign2/report-build-info.html > > Do they match your expectations? It looks like a few testcases need to > be adjusted, > or new bugs are uncovered. > > If the patch OK with a suitable ChangeLog entry? So, I wasn't sure what you meant by the comment. Below is an example of ! working. % proc b {} { return 0 } % set v [expr ![b]] 1 % set v [ expr !![b]] 0 % proc b {} { return 1 } % set v [expr ![b]] 0 % set v [ expr !![b]] 1 Given that, if you want to use a simple set var value, you should just do that directly. [b] is the placeholder for an [] expression, if you want to invert that. v is the placeholder for the thing you want to set. All the other bits should be used as given. Given that code, I'd be interested in what you want to put in the comment, if any. set et_vect_hw_misalign_saved($et_index) [expr ![check_effective_target_arm_vect_no_misalign]] seems like what you want, does that work?
On 25 September 2017 at 20:19, Mike Stump <mikestump@comcast.net> wrote: > On Sep 23, 2017, at 10:52 AM, Christophe Lyon <christophe.lyon@linaro.org> wrote: >> The attached patch would apply after reverting yours. >> I've applied it against r253072 (just before your patch) and the >> results are visible at: >> http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/253072-hw-misalign2/report-build-info.html >> >> Do they match your expectations? It looks like a few testcases need to >> be adjusted, >> or new bugs are uncovered. >> >> If the patch OK with a suitable ChangeLog entry? > > So, I wasn't sure what you meant by the comment. Below is an example of ! working. > > % proc b {} { return 0 } > % set v [expr ![b]] > 1 > % set v [ expr !![b]] > 0 > % proc b {} { return 1 } > % set v [expr ![b]] > 0 > % set v [ expr !![b]] > 1 > > Given that, if you want to use a simple set var value, you should just do that directly. [b] is the placeholder for an [] expression, if you want to invert that. v is the placeholder for the thing you want to set. All the other bits should be used as given. Given that code, I'd be interested in what you want to put in the comment, if any. > > set et_vect_hw_misalign_saved($et_index) [expr ![check_effective_target_arm_vect_no_misalign]] > > seems like what you want, does that work? > Yes, thanks! I was missing the 'expr' part. Here is what I have committed (r253187), to avoid further noise in the results. 2017-09-26 Christophe Lyon <christophe.lyon@linaro.org> * lib/target-supports.exp (check_effective_target_vect_hw_misalign): Fix arm check. * lib/target-supports.exp Index: gcc/testsuite/lib/target-supports.exp =================================================================== --- gcc/testsuite/lib/target-supports.exp (revision 253186) +++ gcc/testsuite/lib/target-supports.exp (working copy) @@ -5951,7 +5951,7 @@ set et_vect_hw_misalign_saved($et_index) 1 } if { [istarget arm*-*-*] } { - set et_vect_hw_misalign_saved($et_index) ![check_effective_target_arm_vect_no_misalign] + set et_vect_hw_misalign_saved($et_index) [expr ![check_effective_target_arm_vect_no_misalign]] } } verbose "check_effective_target_vect_hw_misalign:\ Thanks, Christophe
On Sep 25, 2017, at 9:58 PM, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > Yes, thanks! I was missing the 'expr' part. > > Here is what I have committed (r253187), to avoid further noise in the results. Yup, looks good. Thanks.
Hi All, I'm looking for permission to backport this patch to the gcc-7 branch to fix the failing tests there as well. Thanks, Tamar
On Oct 6, 2017, at 5:48 AM, Tamar Christina <Tamar.Christina@arm.com> wrote: > > I'm looking for permission to backport this patch to the gcc-7 branch to fix the failing tests there as well. Ok.
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 2e0e4d43bfad5b41693539f10425c53e7c418a8f..b7fe5c0d724df0a2965ab82bcb563271a55783df 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -5951,7 +5951,7 @@ proc check_effective_target_vect_hw_misalign { } { set et_vect_hw_misalign_saved($et_index) 1 } if { [istarget arm*-*-*] } { - set et_vect_hw_misalign_saved($et_index) [check_effective_target_arm_vect_no_misalign] + set et_vect_hw_misalign_saved($et_index) ![check_effective_target_arm_vect_no_misalign] } } verbose "check_effective_target_vect_hw_misalign:\