diff mbox series

[committed,testsuite] Re-enable pr94600-{1,3}.c tests for arm

Message ID b74802ae-d5df-4602-4957-75863b467dbb@suse.de
State New
Headers show
Series [committed,testsuite] Re-enable pr94600-{1,3}.c tests for arm | expand

Commit Message

Tom de Vries Sept. 30, 2020, 12:25 p.m. UTC 
[ was: Re: [committed][testsuite] Require non_strict_align in
pr94600-{1,3}.c ]

On 9/30/20 4:53 AM, Hans-Peter Nilsson wrote:
> On Thu, 24 Sep 2020, Tom de Vries wrote:
> 
>> Hi,
>>
>> With the nvptx target, we run into:
>> ...
>> FAIL: gcc.dg/pr94600-1.c scan-rtl-dump-times final "\\(mem/v" 6
>> FAIL: gcc.dg/pr94600-1.c scan-rtl-dump-times final "\\(set \\(mem/v" 6
>> FAIL: gcc.dg/pr94600-3.c scan-rtl-dump-times final "\\(mem/v" 1
>> FAIL: gcc.dg/pr94600-3.c scan-rtl-dump-times final "\\(set \\(mem/v" 1
>> ...
>> The scans attempt to check for volatile stores, but on nvptx we have memcpy
>> instead.
>>
>> This is due to nvptx being a STRICT_ALIGNMENT target, which has the effect
>> that the TYPE_MODE for the store target is set to BKLmode in
>> compute_record_mode.
>>
>> Fix the FAILs by requiring effective target non_strict_align.
> 
> No, that's wrong.  There's more than that at play; it worked for
> the strict-alignment targets where it was tested at the time.
> 

Hi,

thanks for letting me know.

> The test is a valuable canary for this kind of bug.  You now
> disabled it for strict-alignment targets.
> 
> Please revert and add your target specifier instead, if you
> don't feel like investigating further.

I've analyzed the compilation on strict-alignment target arm-eabi, and
broadened the effective target to (non_strict_align ||
pcc_bitfield_type_matters).

Thanks,
- Tom

Comments

Hans-Peter Nilsson Oct. 1, 2020, 5:38 a.m. UTC | #1 
On Wed, 30 Sep 2020, Tom de Vries wrote:

> [ was: Re: [committed][testsuite] Require non_strict_align in
> pr94600-{1,3}.c ]
>
> On 9/30/20 4:53 AM, Hans-Peter Nilsson wrote:
> > On Thu, 24 Sep 2020, Tom de Vries wrote:
> >
> >> Hi,
> >>
> >> With the nvptx target, we run into:
> >> ...
> >> FAIL: gcc.dg/pr94600-1.c scan-rtl-dump-times final "\\(mem/v" 6
> >> FAIL: gcc.dg/pr94600-1.c scan-rtl-dump-times final "\\(set \\(mem/v" 6
> >> FAIL: gcc.dg/pr94600-3.c scan-rtl-dump-times final "\\(mem/v" 1
> >> FAIL: gcc.dg/pr94600-3.c scan-rtl-dump-times final "\\(set \\(mem/v" 1
> >> ...
> >> The scans attempt to check for volatile stores, but on nvptx we have memcpy
> >> instead.
> >>
> >> This is due to nvptx being a STRICT_ALIGNMENT target, which has the effect
> >> that the TYPE_MODE for the store target is set to BKLmode in
> >> compute_record_mode.
> >>
> >> Fix the FAILs by requiring effective target non_strict_align.
> >
> > No, that's wrong.  There's more than that at play; it worked for
> > the strict-alignment targets where it was tested at the time.
> >
>
> Hi,
>
> thanks for letting me know.
>
> > The test is a valuable canary for this kind of bug.  You now
> > disabled it for strict-alignment targets.
> >
> > Please revert and add your target specifier instead, if you
> > don't feel like investigating further.
>
> I've analyzed the compilation on strict-alignment target arm-eabi, and

An analysis should result in more than that statement.

> broadened the effective target to (non_strict_align ||
> pcc_bitfield_type_matters).

That's *also* not right.  I'm guessing your nvptx fails because
it has 64-bit alignment requirement, but no 32-bit writes.
...um, no that can't be it, nvptx seems to have them.  Costs?
Yes, probably your #define MOVE_RATIO(SPEED) 4.

The writes are to 32-bit aligned addresses which gcc can deduce
(also for strict-alignment targets) because it's a literal,
where it isn't explicitly declared to be attribute-aligned

You should have noticed the weirness in that you "only" needed
to tweak pr94600-1.c and -3.c, not even pr94600-2.c, which
should be the case if it was just the test-case getting the
predicates wrong.  This points at your MOVE_RATIO, together with
middle-end not applying it consistently for -2.c.

Again, please just skip for nvptx (don't mix-n-match general
predicates) unless you really look into the reason you don't get
6 single 32-bit-writes only in *some* of the cases.

brgds, H-P
diff mbox series

Patch

[testsuite] Re-enable pr94600-{1,3}.c tests for arm

Before commit 7e437162001 "[testsuite] Require non_strict_align in
pr94600-{1,3}.c", some tests were failing for nvptx, because volatile stores
were expected, but memcpy's were found instead.

This was traced back to this bit in compute_record_mode:
...
  /* If structure's known alignment is less than what the scalar
     mode would need, and it matters, then stick with BLKmode.  */
  if (mode != BLKmode
      && STRICT_ALIGNMENT
      && ! (TYPE_ALIGN (type) >= BIGGEST_ALIGNMENT
            || TYPE_ALIGN (type) >= GET_MODE_ALIGNMENT (mode)))
    {
      /* If this is the only reason this type is BLKmode, then
         don't force containing types to be BLKmode.  */
      TYPE_NO_FORCE_BLK (type) = 1;
      mode = BLKmode;
    }
...
which got triggered for nvptx, but not for x86_64.

The commit disabled the tests for non_strict_align effective target, but
that had the effect for the arm target that those tests were disabled, even
though they were passing before.

Further investigation in compute_record_mode shows that the if-condition
evaluates to false for arm because, because TYPE_ALIGN (type) == 32, while
it's 8 for nvptx.  This again can be explained by the
PCC_BITFIELD_TYPE_MATTERS setting, which is 1 for arm, but 0 for nvptx.

Re-enable the test for arm by using effective target
(non_strict_align || pcc_bitfield_type_matters).

Tested on arm-eabi and nvptx.

gcc/testsuite/ChangeLog:

2020-09-30  Tom de Vries  <tdevries@suse.de>

	* gcc.dg/pr94600-1.c: Use effective target
	(non_strict_align || pcc_bitfield_type_matters).
	* gcc.dg/pr94600-3.c: Same.

---
 gcc/testsuite/gcc.dg/pr94600-1.c | 4 ++--
 gcc/testsuite/gcc.dg/pr94600-3.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/pr94600-1.c b/gcc/testsuite/gcc.dg/pr94600-1.c
index 38f939a98cb..c9a7bb9007e 100644
--- a/gcc/testsuite/gcc.dg/pr94600-1.c
+++ b/gcc/testsuite/gcc.dg/pr94600-1.c
@@ -32,5 +32,5 @@  foo(void)
 }
 
 /* The only volatile accesses should be the obvious writes.  */
-/* { dg-final { scan-rtl-dump-times {\(mem/v} 6 "final" { target { non_strict_align } } } } */
-/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 6 "final" { target { non_strict_align } } } } */
+/* { dg-final { scan-rtl-dump-times {\(mem/v} 6 "final" { target { non_strict_align || pcc_bitfield_type_matters } } } } */
+/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 6 "final" { target { non_strict_align || pcc_bitfield_type_matters } } } } */
diff --git a/gcc/testsuite/gcc.dg/pr94600-3.c b/gcc/testsuite/gcc.dg/pr94600-3.c
index e8776fbdb28..ff42c7db3c6 100644
--- a/gcc/testsuite/gcc.dg/pr94600-3.c
+++ b/gcc/testsuite/gcc.dg/pr94600-3.c
@@ -31,5 +31,5 @@  foo(void)
 }
 
 /* The loop isn't unrolled. */
-/* { dg-final { scan-rtl-dump-times {\(mem/v} 1 "final" { target { non_strict_align } } } } */
-/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 1 "final" { target { non_strict_align } } } } */
+/* { dg-final { scan-rtl-dump-times {\(mem/v} 1 "final" { target { non_strict_align || pcc_bitfield_type_matters } } } } */
+/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 1 "final" { target { non_strict_align || pcc_bitfield_type_matters } } } } */