On Thu, Jul 29, 2021 at 4:38 PM Andrew MacLeod <amacl...@redhat.com> wrote:
>
> On 7/29/21 3:19 AM, Richard Biener wrote:
> > On Wed, Jul 28, 2021 at 4:39 PM Andrew MacLeod <amacl...@redhat.com> wrote:
> >>
> >> Which has removed the second call to builtin_abort() (Even before we
> >> get to EVRP!)
> >>
> >> SO the issue doesn't seem to be removing the divide by 0, it seems to be
> >> a pattern match for [0,1] that is triggering.
> >>
> >> I would argue the test case should not be testing for not removing the
> >> divide by 0... Because we can now fold c_14 to be 486097858, and I
> >> think that is a valid transformation? (assuming no non-call exceptions
> >> of course)
> > I think it's a valid transform, even with -fnon-call-exceptions when
> > -fdelete-dead-exceptions is enabled (like for Ada and C++).
> >
> > You'd have to dig into history to tell why we added this testcase in that
> > way.
> >
> > Richard.
>
> Looks like the PR was adding an optimization to simplification which
> checked to see if the LHS of a binop is a constant and the RHS of the
> binop was a 2-value range.
>
> If so, it tries folding each range individually and if that is
> successful, turns it into a CONDITION ? LHSopRHS1 : LHSopRHS2
>
> THere are 4 test files, the second one is triggering the failure because
> it deals with / and %, and is specifically checking to make sure the
> invalid cases like / 0 and % 0 are not transformed..
>
> With EVRP now able to do something with both the const and the modulus,
> the operations it is specifically scanning for to make sure it didn't
> transform no longer apply.
>
> In fact, the things it checks for:
>
> /* Dont optimize 972195717 / 0 in function foo. */
> /* { dg-final { scan-tree-dump-times "972195717 / " 1 "evrp" } } */
> /* Dont optimize 972195717 % 0 in function bar. */
> /* { dg-final { scan-tree-dump-times "972195717 % " 1 "evrp" } } */
> /* May optimize in function bar2, but EVRP doesn't perform this yet. */
> /* { dg-final { scan-tree-dump-times "972195715 % " 0 "evrp" } } */
>
>
> The last line was already adjusted once when EVRP got smarter and
> started doing the transformation on CST % 1 : 2
>
> I see a couple of options... we could delete the testcase, but Im not
> sure that is useful.
>
> 1) just change those 1's to 0's and move on like was time last time it
> appear.
>
> 2) also add a check that __builtin_abort is only called once. This would
> then be checking that we do all these transformations:
>
> CST / [0,2] will produce CST/2
> CST % [0, 2] will produce either [0,0] or [1,1] based on CST.
> CST % [1,2] will produce [0,1], but be transformed from the % into the
> ?: form.
>
> Is this acceptable? Test tetscase change would look like:
/* Dont optimize 972195717 / 0 in function foo. */
-/* { dg-final { scan-tree-dump-times "972195717 / " 1 "evrp" } } */
+/* { dg-final { scan-tree-dump-times "972195717 / " 0 "evrp" } } */
the comments are now confusing and should be updated, otherwise I'm
fine with this.
