[PATCH] PR19137: Correct PRE handling of llvm.sadd.with.overflow followed by add.

[Erik Verbruggen]

After talking this through with Owen on irc, I think it's better to revert the changes and handle it differently. So, shall I revert 203553, 203558, and 203574?

-- Erik.


On 25 Mar 2014, at 21:32, Daniel Berlin <dberlin at dberlin.org> wrote:

> 
> 
> 
> On Tue, Mar 25, 2014 at 1:53 AM, Erik Verbruggen <erik.verbruggen at me.com> wrote:
> 
> On 24 Mar 2014, at 22:44, Daniel Berlin <dberlin at dberlin.org> wrote:
> 
>> I'm a bit confused why this is the right fix. 
>> If the result types are different, why did it hash to the same value?
>> Either this does matter, and it shouldn't have hashed to the same value (in which case the right fix is that)
> 
> It does not matter for the calculation. The only thing that is needed when e.g. a mul is replaced by a mul-with-overflow is to extract the product from the result. I could also do that, but...:
> 
> But then they aren't the same :)
> If you can't replace one with the other, without additional insertion, it shouldn't be a leader in the current PRE. 
> 
> It seems like the moral equivalent of the injury scheme SSAPRE used for strength reduction (IE "It's possible to make these two operations equal, at the cost of some additional code insertion").
> 
> I worry you are just going to end up exposing more bugs, since it's really not currently an algorithm meant to handle this (GVN right now is quite simple, so it's fairly low risk ...)
> 
> 
>> Or it doesn't matter, and the right fix is to bitcast/whatever to the right type but still do phi insertion, since the bitcast is correct+free.
> 
> It's an extract, for which I'm not completely sure if it's free. If it isn't, PRE would not only move one instruction around, but increase code-size in the basic block where it found the duplicate instruction.
> 
> Right, this is why it's like the strength reduction scheme. You need something to tell you it requires additional code insertion, and may not be a "cost-free" leader.  Then you need something to decide where insertions should go :)
> 
> That said...:
> 
> 
>> Your fix seems, at least without more info, to essentially say "we value numbered some stuff to the same value numbers, but it turns out they aren't really the same enough".
> 
> Yes, for now. This might sound strange, but it's now causing an assert to fail for some people when building clang. I would like to fix that first. Then I'd like to improve it by lifting the duplicate instruction to the first common dominator of the incoming edges.
> 
> PRE should already be finding the optimal place for it if you let it....
> (though thinking about this, i expect it won't because it punts on phi nodes).
> 
>  
>  
>  
> That would eliminate the need for a phi-node and would reduce code size. It also handles the entry in lib/Target/README.txt starting at line 981.
> 
> Hoisting won't replace the store sinking it's talking about (by definition), it handles one specific case of scalar promotion based store removal. 
>  
> 
> In any case, as long as you are signed up to do the work, i'm okay with this (For what it matters), i do think it needs a slightly different comment explaining what's going on and why it's okay.  The comment right now kinda would just make one wonder why they hash to the same value :)
> 
> 
> -- Erik.
> 
> 
> 
>> On Mon Mar 24 2014 at 9:23:24 AM, Erik Verbruggen <erik.verbruggen at me.com> wrote:
>> One-week-ping!
>> 
>> This fixes a failing assert, which some people run into...
>> 
>> -- Erik.
>> 
>> On 17 Mar 2014, at 10:23, Erik Verbruggen <erik.verbruggen at me.com> wrote:
>> 
>> > When PRE finds a leader for a non-intrinsic (e.g. add), where that
>> > leader is an intrinsic (e.g. llvm.sadd.with.overflow), and the basic
>> > block for the non-intrinsic has multiple incoming edges, then do not
>> > insert a phi-node, because the types will be different.
>> >
>> > The previous version only/accidentally worked for 2 incoming edges. The
>> > testcase is changed to test 3 edges.
>> >
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> 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/20140328/e352d3ee/attachment.html>