[PATCH] aarch64: Don't record hazards against paired insns [PR113356]

[PATCH] aarch64: Don't record hazards against paired insns [PR113356]

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

Hi,

For the testcase in the PR, we try to pair insns where the first has
writeback and the second uses the updated base register.  This causes us
to record a hazard against the second insn, thus narrowing the move
range away from the end of the BB.

However, it isn't meaningful to record hazards against the other insn
in the pair, as this doesn't change which pairs can be formed, and also
doesn't change where the pair is formed (from the perspective of
nondebug insns).

To see why this is the case, consider the two cases:

 - Suppoe we are finding hazards for insns[0].  If we record a hazard
   against insns[1], then range.last becomes
   insns[1]->prev_nondebug_insn (), but note that this is equivalent to
   inserting after insns[1] (since insns[1] is being changed).
 - Now consider finding hazards for insns[1].  Suppose we record
   insns[0] as a hazard.  Then we set range.first = insns[0], which is a
   no-op.

As such, it seems better to never record hazards against the other insn
in the pair, as we check whether the insns themselves are suitable for
combination separately (e.g. for ldp checking that they use distinct
transfer registers).  Avoiding unnecessarily narrowing the move range
avoids unnecessarily re-ordering over debug insns.

This should also mean that we can only narrow the move range away from
the end of the BB in the case that we record a hazard for insns[0]
against insns[1]->prev_nondebug_insn () or earlier.  This means that for
the non-call-exceptions case, either the move range includes insns[1],
or we reject the pair (thus the assert tripped in the PR should always
hold).

Bootstrapped/regtested on aarch64-linux-gnu with/without ldp passes
enabled on top of the PR113070 fixes, OK for trunk?

Thanks,
Alex

gcc/ChangeLog:

	PR target/113356
	* config/aarch64/aarch64-ldp-fusion.cc (ldp_bb_info::try_fuse_pair):
	Don't record hazards against the opposite insn in the pair.

gcc/testsuite/ChangeLog:

	PR target/113356
	* gcc.target/aarch64/pr113356.C: New test.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1180 bytes --]

diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
index 703cfb1228c..6834560c5fb 100644
--- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
+++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
@@ -2216,11 +2216,11 @@ ldp_bb_info::try_fuse_pair (bool load_p, unsigned access_size,
 	  ignore[j] = &XEXP (cand_mems[j], 0);
 
       insn_info *h = first_hazard_after (insns[0], ignore[0]);
-      if (h && *h <= *insns[1])
+      if (h && *h < *insns[1])
 	cand.hazards[0] = h;
 
       h = latest_hazard_before (insns[1], ignore[1]);
-      if (h && *h >= *insns[0])
+      if (h && *h > *insns[0])
 	cand.hazards[1] = h;
 
       if (!cand.viable ())
diff --git a/gcc/testsuite/gcc.target/aarch64/pr113356.C b/gcc/testsuite/gcc.target/aarch64/pr113356.C
new file mode 100644
index 00000000000..0de17a54a53
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr113356.C
@@ -0,0 +1,8 @@
+// { dg-do compile }
+// { dg-options "-Os -fnon-call-exceptions -mearly-ldp-fusion -fno-lifetime-dse -fno-forward-propagate" }
+struct Class1 {
+  virtual ~Class1() {}
+  unsigned Field1;
+};
+struct Class4 : virtual Class1 {};
+int main() { Class4 var1; }

Re: [PATCH] aarch64: Don't record hazards against paired insns [PR113356]

[Richard Sandiford]

Alex Coplan <alex.coplan@arm.com> writes:
> Hi,
>
> For the testcase in the PR, we try to pair insns where the first has
> writeback and the second uses the updated base register.  This causes us
> to record a hazard against the second insn, thus narrowing the move
> range away from the end of the BB.
>
> However, it isn't meaningful to record hazards against the other insn
> in the pair, as this doesn't change which pairs can be formed, and also
> doesn't change where the pair is formed (from the perspective of
> nondebug insns).
>
> To see why this is the case, consider the two cases:
>
>  - Suppoe we are finding hazards for insns[0].  If we record a hazard
>    against insns[1], then range.last becomes
>    insns[1]->prev_nondebug_insn (), but note that this is equivalent to
>    inserting after insns[1] (since insns[1] is being changed).
>  - Now consider finding hazards for insns[1].  Suppose we record
>    insns[0] as a hazard.  Then we set range.first = insns[0], which is a
>    no-op.
>
> As such, it seems better to never record hazards against the other insn
> in the pair, as we check whether the insns themselves are suitable for
> combination separately (e.g. for ldp checking that they use distinct
> transfer registers).  Avoiding unnecessarily narrowing the move range
> avoids unnecessarily re-ordering over debug insns.
>
> This should also mean that we can only narrow the move range away from
> the end of the BB in the case that we record a hazard for insns[0]
> against insns[1]->prev_nondebug_insn () or earlier.  This means that for
> the non-call-exceptions case, either the move range includes insns[1],
> or we reject the pair (thus the assert tripped in the PR should always
> hold).
>
> Bootstrapped/regtested on aarch64-linux-gnu with/without ldp passes
> enabled on top of the PR113070 fixes, OK for trunk?
>
> Thanks,
> Alex
>
> gcc/ChangeLog:
>
> 	PR target/113356
> 	* config/aarch64/aarch64-ldp-fusion.cc (ldp_bb_info::try_fuse_pair):
> 	Don't record hazards against the opposite insn in the pair.
>
> gcc/testsuite/ChangeLog:
>
> 	PR target/113356
> 	* gcc.target/aarch64/pr113356.C: New test.

OK, thanks.

Richard