Address @tejohnson comment by adding NFCs to commit msg and summary.

Ping 2X. Thanks!

collect some numbers on sqlite3.c

LGTM. @efriedma, does this look good to you ? I think we have to have similar treatments for memcpyopt in the past.

This looks good to me. I will let @courbet to have the final say here.

In D43690#1206134, @trentxintong wrote:

In D43690#1206049, @vlad.tsyrklevich wrote:

In D43690#1201965, @twoh wrote:

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

Hi @vlad.tsyrklevich, @twoh

I am working on creating a reduced test case. Will get back to you.
Thanks

In D43690#1206049, @vlad.tsyrklevich wrote:

In D43690#1201965, @twoh wrote:

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

@evgeny777
Thanks!. The approach I am considering is to bring in something, like a stub (e.g. the declaration completely unmapped, we cant map it fully anyways) and eventually using it as a handle to RAUW when we have properly mapped in the SGV. Do you think this would work ?

In D43690#1206049, @vlad.tsyrklevich wrote:

In D43690#1201965, @twoh wrote:

Hello, I wonder if we need to keep linkonce_odr symbols live here as well. I observe a case that a vtable for template class initiated has linkonce_odr linkage and marked dead here, which results compiler crash at WholeProgramDevirt because the global variable for vtable doesn't have initializer (https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/WholeProgramDevirt.cpp#L676 assumes that GV has the initializer). Thanks!

I'm not sure why a linkonce_odr GV would be marked non-prevailing. Do you have a minimized example to look at?

@evgeny777 Do you mind I give this bug a try ? Thanks.

I dumped the Composite right after the 2nd file is merged. Things look good except the DITemplateValueParameter debuginfo looks a bit odd. Does this look correct to you ?

@fhahn Thank you for doing this. This is not a blocking issue for us. But it would be nice to have it fixed (for us and the possibly other users of LLVM in general). If you and @davide agree we should do this before having a real fix. I can write a test and land this. Otherwise, I am fine waiting for the real fix.

Address comments. Thanks @hfinkel

This brings about 20ms - 25ms out of ~20s when compile sqlite3.c

Update diff addressing all of @hfinkel comments.

@hfinkel I get what you mean. Basically moving the IntrinsicInst check inside the call does not result us doing extra work when processing IntrinsicInst as we essentially do the same things in the dyn_cast<IntrinsicInst>. I will update the diff accordingly.

Address some of @hfinkel suggestions.

Thank you for taking a look at this code. I agree we should definitely "else" the StoreInst check and I have observed improvements in Xcode Instrument for doing so.

Friendly Ping. Are there any problems with this patch ? Do I need to collect more #s ?

Hi @brzycki. This is a deficiency in a feature we already have. In jump threading, when we can tell all the predecessors of a block go to the same destination. we do not need to thread, we can just fold the terminator of the block. This has less impact on the CFG and also we do not have the problem of not being able to jump threading because the block can not be duplicated.

Friendly ping.

Address @asbirlea comments

@dmgreen Thanks for the pointer. I think https://reviews.llvm.org/D48202 will define away the problem. I will abandon this patch.

I think this makes sense. Except Jumpthreading needs to be modified to make sure iterator is valid after merging. I can work on the jump threading part once this lands and if you do not mind @brzycki.

Remove some duplicate code.

run clang-format

Handle when all destinations are null (i.e. UNDEF value). There is a slight change in pr22086.ll this is due
to we do not run SimplifyInstructionsInBlock after folding (we run after threading).

I am having some trouble with arc. Created this diff accidentally. Its a duplicate of https://reviews.llvm.org/D48181

@davide @fhahn I am sorry that I cant provide the source code. But one of the cases that resulted in chasing up the chain a lot I can see came from a sequence of object declarations & initializations (constructors which can throw). And they cascaded into a block with 2 predecessors.

Will do.

And I think adding an assert to check against queries w.r.t. dominance makes sense.

@dberlin @reames I completely agree with the diagnosis and the proposed solution. Let me fix this.

Simplify test. I could not get rid of the inttoptr though.

Ping. Thanks.

Remove empty lines.

@davide @fhahn I will take a look!.

In D45608#1067141, @junbuml wrote:

Thanks for handling this. Just curious if you see any issue with the non-null attribute in a constant ?

Address comments.

Address stylistic comments.

@courbet Ping. Thanks

Update checks in a test case.

Simplify how split instructions are moved.

Update an assert.

Address comment.