[Bug ada/68590] [6 Regression] FAIL: gnat.dg/loop_optimization19.adb scan-tree-dump-not optimized "Index_Check"

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68590

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Instead it originates from
> 
> (for cmp (ge le)                                                            
> 
>  (simplify                                                                  
> 
>   (cmp @0 @0)                                                               
> 
>   (eq @0 @0)))
> 
> hmm.  Yes, we use @0 twice here.  But we also use a matching capture, sth
> we (unfortunately) don't record (yet).  We _do_ have a load that we
> compare so avoiding the redundant computation when we duplicate an
> operand makes sense even when there is no side-effect present (that was
> the objective of the change).  Simplifying
> 
>  (le  s1->length s1->length)
> 
> to
> 
>  (eq save_expr <s1->length> save_expr <s1->length>)
> 
> looks like a win.

I beg to differ, s1->length has no TREE_SIDE_EFFECTS so it's a pessimization.
Having no TREE_SIDE_EFFECTS *precisely* means that you are free to reuse the
object as-is in the same expression any number of times.

> Now simplifying that down to
> 
>  (save_expr <s1->length>, 1)
> 
> is "correct" as well.  And we can't just drop random save_exprs here because
> of the comment in save_expr:
> 
>   /* This expression might be placed ahead of a jump to ensure that the
>      value was computed on both sides of the jump.  So make sure it isn't
>      eliminated as dead.  */
>   TREE_SIDE_EFFECTS (t) = 1;
> 
> not sure what this is refering to... it sounds confusing - if it was placed
> there and did not have side-effects it better should have a user via a
> data dependence.

Having TREE_SIDE_EFFECTS on SAVE_EXPRs is required for GENERIC folding (of
course not if your ultimate objective is to kill GENERIC folding...) because if
you have something like:

  COND_EXPR <cond, op0 + op1, op0 - op2>

and want to turn it into:

  COND_EXPR <cond, SAVE_EXPR <op0> + op1, SAVE_EXPR <op0> - op2>

the SAVE_EXPR <op0> will create a variable that will be initialized once, in
other words in only one of the arms and you don't know which one at compile
time.  So you need to create a COMPOUND_EXPR:

  COMPOUND_EXPR <SAVE_EXPR <op0>, COND_EXPR <cond, SAVE_EXPR <op0> + op1,
SAVE_EXPR <op0> - op2>>

to make sure it is evaluated ahead of the branch and TREE_SIDE_EFFECTS is
required to keep the first operand.

I'll note that the problematic change comes with neither a testcase nor a
comment (and the original comment about TREE_SIDE_EFFECT is even still there)
so it's little hard to understand where it comes from...