[PATCH] Infer known bits from dominating conditions

Philip Reames listmail at philipreames.com
Tue Mar 3 11:06:10 PST 2015

On 03/03/2015 10:39 AM, hfinkel at anl.gov wrote:
>> To be clear, is this a request for me to evaluate this before submission?  I really don't see the need given that a) the current implementation is fast, and b) this is an experimental flag.
> Yes, it is. My feeling is that this is not particularly complicated, and if it is, then there's something I'm not considering properly.
> To be clear, this is a very important missing capability, and I want to make sure that we don't prematurely optimize here:  inconsistencies here will be *very* user visible, and I'm concerned that with your approach, small local code changes will cause drastic changes in the output by turning on and off this logic. If a more general approach is not feasible, then so be it, I'll take what we can get, but I want to make his decision quantitatively -- and I don't want to start with a design that will need to be rewritten to make forward progress.
Ok, let's take a step back for a second.  I think we've slipped into an 
argument when we actually agree.

I think all of the issues you raised should be evaluated and addressed.  
I'm even willing to do most of the work to do so. However, I would 
really strongly prefer doing that *in tree*.  You have access to 
workloads I don't.  So does Nick.  So do other interested parties.  What 
I want is to check in a shared implementation (or two, or three), and 
give each of us a chance to measure on our workloads.  Running with a 
set of flags is generally much easier than building a custom build with 
a potentially tweaked patch and keeping track of which patch generated 
which set of results.

Now, on the technical side again, I think we're coming from two 
different perspectives.  My overwhelming concern is compile time. We've 
tried various attempts at value property propagation in the past and all 
of them have failed due to that issue.  What I'm aiming to do is find 
*something* which can at least catch the simple cases and can be turned 
on without impacting performance.  I'd like to push that as far as 
possible, but that's a second order concern. You seem to be focused more 
on the effectiveness side, with a second order interest in performance.  
Given past history, I don't think that's the right approach to take here.

Also, can you give an example of a code change that concerns you? At 
least on the surface this seems pretty stable to me.  Early exits from 
functions and loops gives you information the optimizer can exploit.  
Anything else might, but might not.  That seems entirely understandable.
>>> Why do you care that the cmp has only one use? Why not look at all of the branches that use the cmp result?
>>   We could, and may someday.  Doing so involves a potential increase in
>>   compile time.  I'm deliberately going for minimal compile time impact,
>>   even at the loss of some effectiveness.  I want to be able to turn this
>>   on by default.  Most of my actual motivating cases are really simple.
> Please measure this. Unless this is really needed, please take it out. Users can change codes to introduce extra uses of comparison results in many ways, and doing so should not change the optimizers ability to make use of the information.
>> I would strongly prefer going with the fastest implementation now and
>>   evaluating extensions at a later time.  If you think it matters and is
>>   cheap enough, feel free to contribute a patch.  :)
> But I don't know that it is the fastest implementation, or how much that matters. Fast matters, but it is not the only thing that matters. Using a DT walk with a cutoff might actually be faster. Stability of optimization results to small source changes is also really important. Consistency with other optimizer capabilities is also really important.
> Let's work together on this. Yes, if you don't want to do this, I'll get to it in a couple of months.
> http://reviews.llvm.org/D7708
>    http://reviews.llvm.org/settings/panel/emailpreferences/

More information about the llvm-commits mailing list