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

[Hal Finkel]

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

 -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