Re: [Patch RFC IA64] Fixing predication & autoinc interaction

[Alexander Monakov <amonakov at ispras dot ru>]

Hi,

On Thu, 24 Sep 2009, Steve Ellcey wrote:

> I would like to get some feedback on this proposed patch to fix an IA64
> bug that is caused by the interaction of predication and autoincrement.
>
> The problem is that GCC is generating predicated instructions that
> use autoincrement but if the predicated instruction is not executed
> then the autoincrement does not happen and we get incorrect results.

Well, such transformation just should not be applied.  Any pass that is 
producing/modifying predicated code should make sure, among other things, 
that execution of side effects, like post increments, is preserved.  For 
example, GCC's if-conversion pass already has precautions against bringing 
POST_INCs inside COND_EXECs (in function check_cond_move_block, 
specifically testing 'side_effects_p ({src,dest})').

I have looked into testcase attached to PR41365, and miscompilation there 
is caused by the combine pass, not if-conversion.  It modifies existing 
predicated code by bringing a load with post-increment inside a 
conditional assignment.  I think such case is not covered by try_combine's 
long sequence of correctness testing, so I suggest adding it.  I have 
started bootstrap/regtest of the following patch, which fixes the problem:

--cut
diff --git a/gcc/combine.c b/gcc/combine.c
index 35ab576..e90d2a4 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2803,6 +2803,17 @@ try_combine (rtx i3, rtx i2, rtx i1, int *new_direct_jump_p)
        }
 #endif

+  /* Do not bring expressions with side-effects inside conditional code.  */
+  if ((GET_CODE (PATTERN (i3)) == COND_EXEC
+       || (GET_CODE (PATTERN (i3)) == SET
+          && GET_CODE (SET_SRC (PATTERN (i3))) == IF_THEN_ELSE))
+      && ((i1 && side_effects_p (PATTERN (i1)))
+         || side_effects_p (PATTERN (i2))))
+    {
+      undo_all ();
+      return 0;
+    }
+
   /* See if the SETs in I1 or I2 need to be kept around in the merged
      instruction: whenever the value set there is still needed past I3.
      For the SETs in I2, this is easy: we see if I2DEST dies or is set in I3.
--cut

> My fix is to break the instructions that allow both predication and
> autoincrement modes into two insructions, one that does not allow
> autoincrement (but is otherwise identical) and one that allows
> autoincrement but has the "predicable" attibute set to "no" so that
> it won't be used as a predicated instruction.  This seems to be working
> but I thought I would see if anyone had any comments on my approach
> or ideas on a better way to do this.

If I understand the situation correctly, the problem is not in the machine 
description, but rather in the optimization passes (combine in this case). 
Restricting the machine description may just hide the problem, which may 
later resurface on other targets supporting predicated post-incs.  So, if 
I'm not mistaken with the analysis, it's seems better to keep the machine 
description as is and fix optimizers.

> I haven't yet addressed the speculative and advanced loads but I think
> I need to do those too.  Can they be used as predicated instructions?

Yes, they can.  However, I think GCC does not currently generate them.

--
Alexander Monakov