Address comments from @davidxl

Update comment in test case.

Address @tejohnson comment. Thanks!

Add the same thing to the new pass manager as suggested by @tejohnson

Thanks for the change @fhahn. I will dig up my test case and run it.

This is not an issue. Thanks @fhahn.

Update last bit of suggestions. Thanks for review @tejohnson.

Address @tejohnson suggestions.

Address @tejohson's suggestion to add test cases.

Sure. I will add those tests. Thanks for spending time reviewing this @tejohnson!

Address @tejohnson suggesions. Thanks!

Address @tejohnson suggestions. Thanks!

Internalization leads to the possibility of changing the function ABI, better global modref AA, better chance of discarding an unused symbol, etc.

D51550 has been accepted.

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