On Thu, May 10, 2018 at 1:41 AM, Hans-Peter Nilsson
<hans-peter.nils...@axis.com> wrote:
[...]
With all the followups this generated I'm still going to reply here.
> Now a rant on the match.pd ":s" flag, which reasonable people
> may reasonably suggest I should have used in the patch instead
> of the (if (single_use ...)).
>
> Initially, I got the match.pd-language (which deserves a proper
> name) all wrong. Then I read the documentation and still got it
> wrong. I "misunderstood" that the ":s" on an operand "O" was
> supposed to have the effect of conditionalize the replacement
> "R" by wrapping it in "(if (single_use (O)) R)" as in the
> suggested patch (above). To wit, this does not work; it will
> *not* stop the replacement as seen in the test-case (THIS IS NOT
> A SUGGESTED PATCH):
>
> (for div (trunc_div exact_div)
> (simplify
> - (div (div @0 INTEGER_CST@1) INTEGER_CST@2)
> + (div (div:s @0 INTEGER_CST@1) INTEGER_CST@2)
> (with {
> bool overflow_p;
> wide_int mul = wi::mul (wi::to_wide (@1), wi::to_wide (@2),
>
> In PR69556, it seems other people seem to have read the
> documentation of ":s" the same way, but are corrected by other
> comments there, so I guess it's not my reading that's flawed.
The documentation doesn't mention a few details, yes ... I will
amend it.
> I suggest preferably (1) correcting the semantics of ":s" to do
> as the documentation says because I don't understand the
> explanation in PR69556 comment #4 that the replacement "is still
> allowed if it is a single operation as that replaces at least
> one other (the one we are simplifying)"; I see that as a
> complete nullification of the :s flag, making it a nop.
It doesn't make it a nop in all cases. One motivation of the
match.pd language was to allow pattern-matching and simplification
in during value-numbering in an efficient way. So when I originally
added :s it failed to detect equivalences it could otherwise handle,
like computing that (x / 5) / 6 is equal to x / 30 because when
one (rightfully so!) adds :s to the x / 5 then for the purpose of
canonicalization (which is what value-numbering is interested in)
the "simplification" no longer happens. So based on what
our value-numbering implementation handles I restricted :s to
be in effect only if the simplification causes extra expressions
beside the toplevel one to be emitted. Marc gave good examples
for the (rdiv (rdiv:s ...) ...) pattern.
That's not perfect and we've added explicit single_use () checks
in (too many) places.
Most issues pop up with stmt-local foldings which indeed should
better treat :s "literally". That's because we are lacking a stmt
folder with a global cost model - if you have two expressions like
tem = x/5; (tem / 7) and (tem / 8) then you _do_ want to simplify
them to x / 35 and x / 40 because tem becomes dead.
I suppose we need a way to make :s behave depending on
context (value numbering or stmt folding). The current
implementation doesn't make that too easy - in fact the
current semantic is very simple in the implementation.
I also think that if the result evaluates to a non-operation
(constant or operand) we do not want to "honor" :s either.
> Alternatively (2), if the :s is *not* a nop in some case add an
> example of that case and sufficient explanation to
> match-and-simplify.texi *and* the suggested ":S" flag
> (i.e. *really* conditionalize the replacement by a single-use of
> that operand).
I didn't want to add :S. Beacuse it's not about :s or :S it's really
about the context in which the patterns are used. In some
sense :s should even be implicitely present on all expressions
since originally we just moved fold-const.c patterns to match.pd
and there's no such thing as multi-uses in GENERIC (ok, only if you
ignore SAVE_EXPR).
That said, without actually reviewing the patch, there's room for
improvement. That is not adding :S. What it exactly is remains
to be determined ;)
Richard.
> That's just a detail, I do like that language.
>
> brgds, H-P
