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

[Erik Verbruggen]

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

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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140325/ff9ce10f/attachment.html>