[PATCH] [InstCombineAddSub] Added the transformation - "sub (or A B) (xor A B) -> (and A B)"

[David Majnemer]

On Sat, Oct 18, 2014 at 9:37 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
> > From: "David Majnemer" <david.majnemer at gmail.com>
> > To: "ankur29 garg" <ankur29.garg at samsung.com>, dexonsmith at apple.com,
> "suyog sarda" <suyog.sarda at samsung.com>, "david
> > majnemer" <david.majnemer at gmail.com>
> > Cc: llvm-commits at cs.uiuc.edu
> > Sent: Saturday, October 18, 2014 4:47:12 AM
> > Subject: Re: [PATCH] [InstCombineAddSub] Added the transformation - "sub
> (or A        B) (xor A B) -> (and A B)"
> >
> > InstCombine is not supposed to handle every possible just because it
> > is theoretically possible case.  It's engineered to be a series of
> > engineering tradeoffs.  I don't know what the history of the similar
> > transform is in VisitAdd, it's possible that it wasn't added in a
> > principled way.
> >
> > If we were to arbitrarily add patterns, we would eventually make
> > InstCombine cripplingly slow.
>
> To be honest, I think this position is unhelpful. A few thoughts:
>
>  - In my experience (derived from my work on an instruction-level
> superoptimizer), it is really hard to predict what patterns will occur in
> practice -- and given knowledge of certain bits from switch statements,
> enums (range metadata), etc. combined with LLVM's canonicalization in terms
> of large integers, and several other factors, lead to many patterns that I
> would consider strange, and not-uncommonly in performance-sensitive regions
> of code.


Sure, it is very hard to predict what is and is not useful.  But say, self
hosting LLVM, building a large project like Chrome, an OS kernel or some
benchmark like Spec would tell us what would happen with a particular
corpus.  If we never know that the transform fires, why have it?


>
>  - We (obviously) should not engage in premature optimization -- I've seen
> no evidence that matching these kinds of patterns in InstCombine
> contributes a significant portion of its running time. And until such
> evidence is presented, we should not make statements about expense (there
> is no complexity argument here). In any case, looking at combinations of
> three binary operators, are there any more than a few hundred that could
> simplify in any reasonable sense? Probably not even that many. I see no
> reason we should not handle them all.


I view this sort of change as a hypothetical improvement which is more or
less benign, it doesn't really hurt or help InstCombine.  Perhaps I'm just
afraid of "death by a thousand cuts", maybe *I* am engaging in premature
optimization by thinking along these lines.


>
>  - For simplifications that require certain bits to be known, calls into
> ValueTracking can be expensive, but making duplicate calls into
> ValueTracking is a separate issue that can be fixed if it becomes important.
>
>  - I feel that, in the past, the real engineering tradeoff was slightly
> different: it used to be difficult to prove the transformations correct. As
> a result, there was a significant possibility of bugs being introduced by
> each of these transformations, and it made more sense to limit them. As you
> know, this is no longer anywhere near as true as it was: we now have good
> tools to verify these transformations.
>

It's still true though.  Verifying that a single combine doesn't tell us if
the combine is safe.  I have personally fixed combines which resulted in
infinite loops because the transform would conflict with some other
transform.
Both transforms, when formulated for Z3/Alive/whatever, would be "correct"
but are obviously wrong in the greater context of InstCombine.  Every
change carries some risk, this change is very low risk but it provides no
known benefit.

Some of my concern lies with adding transforms which make the
canonicalization of IR go one way or the another when in practice we don't
actually care because the IR is not known to ever occur in the real world.
However, we now have to keep yet another transform in mind when determining
if a new transform is sound.

In the greater scheme of things, Rome won't fall if we add this transform.
In fact, I think it's more reasonable than not to have this particular
transform.  I just don't think the bar used to judge a new InstCombine
should be so low as to roll around on the ground.


> Thanks again,
> Hal
>
> >
> > http://reviews.llvm.org/D5719
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141018/f4d1dab0/attachment.html>