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

[David Majnemer]

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

> ----- Original Message -----
> > From: "Hal Finkel" <hfinkel at anl.gov>
> > To: reviews+D5719+public+3088893b38cafcf5 at reviews.llvm.org
> > Cc: "ankur29 garg" <ankur29.garg at samsung.com>, "suyog sarda" <
> suyog.sarda at samsung.com>, llvm-commits at cs.uiuc.edu
> > Sent: Saturday, October 18, 2014 11:37:40 PM
> > Subject: Re: [PATCH] [InstCombineAddSub] Added the transformation - "sub
> (or  A       B) (xor A B) -> (and A B)"
> >
> > ----- 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.
>
> To add another anecdote, just when I thought I had classified the kinds of
> patterns that were occurring in practice, I started looking at instrumented
> code (address sanitizer, etc.), and I learned that I had a lot left to do.
>

I think that makes an excellent argument for looking at what happens in
practice and focusing optimizations to improve those circumstances.


>
>  -Hal
>
> >
> >  - 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.
> >
> >  - 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.
> >
> > 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
> > _______________________________________________
> > 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141018/229934c2/attachment.html>