diff mbox

[03/05] Fix PR 66660

Message ID alpine.LNX.2.20.1603142019180.32612@monopod.intra.ispras.ru
State New
Headers show

Commit Message

Alexander Monakov March 14, 2016, 5:37 p.m. UTC 
On Mon, 14 Mar 2016, Andrey Belevantsev wrote:
> We speculate an insn in the PR but we do not make a check for it though we
> should.  The thing that broke this was the fix for PR 45472.  In that pr, we
> have moved a volatile insn too far up because we failed to merge the bits
> describing its volatility when we have processed a control flow split.  The
> code to propagate the insn pattern with the insn merging was added when the
> volatility of the two insns from the both split branches differ. However, the
> volatility of the speculated insn and its original differ: the original insn
> may trap while the speculated version may not.  Thus, we replace a speculative
> pattern with the original one per the PR 45472 fix for no reason.
> 
> The patch for this problem just limits the original fix for PR 45472 to apply
> for non-speculative insns only.  There is no test as it is not so easy to
> construct one -- we could count the number of speculation check in the
> resulting assembly but there is no way to force speculation to happen.
> 
> Ok for trunk?
> 
> gcc/
> 
> 2016-03-14  Andrey Belevantsev  <abel@ispras.ru>
> 
>     PR target/66660
>     * sel-sched-ir.c (merge_expr): Do not propagate trap bits into 
> speculative insns.

I think this line doesn't capture the issue at hand well; the issue is not in
propagating trap bits, but rather unintentionally dropping the speculative
pattern, right?  I'd be happier with something like "If the pattern is already
speculative, keep it, and do not check trap bits".

seems the upper comment matches the code better now, while the lower one could
be improved to say that may_trap_p is deliberately ignored when 'to' is
already speculated.

Finally, I'd recommend to switch around the two VINSN_MAY_TRAP_P tests so that
condition on 'to' is consistently checked prior to condition on 'from'.

OK with those changes.

Thanks.
Alexander
diff mbox

Patch

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index e181cb9..ec59280 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -1871,12 +1871,12 @@  merge_expr (expr_t to, expr_t from, insn_t split_point)
   /* Make sure that speculative pattern is propagated into exprs that
      have non-speculative one.  This will provide us with consistent
      speculative bits and speculative patterns inside expr.  */
-  if ((EXPR_SPEC_DONE_DS (from) != 0
-       && EXPR_SPEC_DONE_DS (to) == 0)
-      /* Do likewise for volatile insns, so that we always retain
-	 the may_trap_p bit on the resulting expression.  */
-      || (VINSN_MAY_TRAP_P (EXPR_VINSN (from))
-	  && !VINSN_MAY_TRAP_P (EXPR_VINSN (to))))
+  if (EXPR_SPEC_DONE_DS (to) == 0
+      && (EXPR_SPEC_DONE_DS (from) != 0
+	  /* Do likewise for volatile insns, so that we always retain
+	     the may_trap_p bit on the resulting expression.  */
+	  || (VINSN_MAY_TRAP_P (EXPR_VINSN (from))
+	      && !VINSN_MAY_TRAP_P (EXPR_VINSN (to)))))
     change_vinsn_in_expr (to, EXPR_VINSN (from));

The patch looks unusual in that it reshuffles code while keeping comments; it