ping

ping

LGTM. Could you please add a unittest similar to the one added in D40985?

In D52307#1241165, @efriedma wrote:

An alloca can have multiple associated lifetime start/ends. Inlining routinely generates code like that, unrolling generates code like that, etc. And stack coloring knows how to handle such code.

Granted, the way the intrinsic is specified makes everything related to it pretty confusing.

In D52312#1240764, @dexonsmith wrote:

I'm concerned about propagating this pattern. DenseSet, like DenseMap, is tuned for small keys and values. SmallVector<void*,4> is not exactly small, at 48B. And this patch makes it easy to throw much larger SmallVectors into these containers.

In D52307#1240955, @efriedma wrote:

Duplicating lifetime.start and lifetime.end intrinsics isn't fundamentally a problem, as long as they stay correctly paired. What is this supposed to solve?

Move the lifetime handling before cloning the instruction. The cases where we delete the clone do not apply to lifetime intrinsics anyways, so there is no need to clone it in that case.

Rebased and added comments.

I think this fixes a longstanding bug report PR15376.

LGTM thanks! It would be great if you could also add a simple test case.

LGTM, with my previous comments.

I simplified it. I am not sure how I can get rid of va_list; what i am testing is va_start calledin the caller and va_end in the callee. I need to have va_list to pass it from the caller to the callee.

It's probably worth waiting with committing until tomorrow, in case Eli has any more suggestions

LGTM

ping

ping

Further testing surfaced a few cases where ScalarEvolution and LCSSA are not preserved properly. I'll fix those issues first.

FWIW, it seems like OrderedBB invalidation is causing bugs at least in LoopSafetyInfo (D50377) which @mkazantsev is currently working on fixing. There might be other places that get this wrong too, so having automatic invalidation seems like another good plus on top of the speedups.

Thanks Sjoerd!

ping

Strengthen test checks

Thanks! I've rebased the patch and will wait for a bit, in case there are any additional concerns, please let me know.

D51664 gets rid of building OrderedBasicBlocks and comes with its own validation of the ordering.

ping

Thank you very much for having a look so quickly!

Adjusted comment and simplified test case.

This is not ready for review yet, I'll continue with this if D51505 is submitted.

IIUC by always adding to the end of WorkListVec, we could end up using substantial more memory, depending on the input program. In practice it shouldn't be too bad, as I think an upper bound for the times an instruction is added to WorkListVec is the number of operands (it will only get added if one of its operands gets constant folded, and once an operand is constant folded, it won't get folded again). And usually most instructions have a small number of operands.

In D51169#1225675, @bjope wrote:

An alternative could perhaps be to do something like this

Changed |= !InstructionsToErase.empty();

before the loop (assuming that if we got something in InstructionsToErase then we also have done some modifications).
But I think your solution (setting Changed whenever we do a change) is easier to understand.

Thanks for the patch. LGTM!

Rebased after rL341537

ping

Add comment

LGTM with some more nits to the test cases. The EnableVPlanNativePath sprinkled around are not ideal, but I don't think there is a better alternative and we should be able to get rid of them as we go along. Please wait a few days with submitting this, to give other people a change to raise any additional concerns.

ping

ping

ping

In D50820#1223015, @fhahn wrote:

I just tried to build your patch, and it triggers

lib/Transforms/Vectorize/LoopVectorize.cpp:1288: bool llvm::LoopVectorizationCostModel::isScalarAfterVectorization(llvm::Instruction*, unsigned int) const: Assertion `ScalarsPerVF != Scalars.end() && "Scalar values are not calculated for VF"' failed.

for the vplan tests.

I just tried to build your patch, and it triggers

lib/Transforms/Vectorize/LoopVectorize.cpp:1288: bool llvm::LoopVectorizationCostModel::isScalarAfterVectorization(llvm::Instruction*, unsigned int) const: Assertion `ScalarsPerVF != Scalars.end() && "Scalar values are not calculated for VF"' failed.

Great, thanks for the review!

Ok thanks! So I can commit this patch as is?

In D51435#1219433, @spatel wrote:

Comment + extra tests look good.

Given the revert of D49273, make the getSqrtCall() diff an NFC commit ahead of this patch + generalize that for any libcall, so we make sure that we're not generating libcalls when we're not allowed to?

updated merging of unknown and notconstant values to go to overdefined

Thanks Eli!

In D51431#1217705, @xbolva00 wrote:

Does this patch + https://reviews.llvm.org/D50039 optimizes following code?

int l = strlen(s);
if (!s) <-- remove this condition

return -1;

return len;

ping

ping