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

[Hal Finkel]

----- Original Message -----
> From: "David Majnemer" <david.majnemer at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D5719+public+3088893b38cafcf5 at reviews.llvm.org, "ankur29 garg" <ankur29.garg at samsung.com>, "suyog sarda"
> <suyog.sarda at samsung.com>, "LLVM Commits" <llvm-commits at cs.uiuc.edu>
> Sent: Sunday, October 19, 2014 12:48:41 AM
> Subject: Re: [PATCH] [InstCombineAddSub] Added the transformation - "sub (or A B) (xor A B) -> (and A B)"
> 
> 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.
> 
> 

To a point, I agree. But it is based on this experience that I had the following reaction to this thread: a generic three -> one transformation involving sub, or and xor? Yea, that'll happen ;)

 -Hal

> 
> -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
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory