@RKSimon sorry for so much back-and-forth.

Can you please submit this upstream first?

@fwolff reverse ping?
This doesn't need any big fundamental changes, just a little rebasing.

Misc wording improvements.

In D114832#3177534, @davidxl wrote:

The bad code pattern generated is probably not general enough to be useful to introduce a cleanup pass or to enhance existing pass to do so -- it will probably just shift the complexity from one pass to another. Fixing this at the source (SROA) is reasonable (unlike other canonicalization pass, this code pattern from SROA actually makes IR much worse)

Some more blurb.

Drop nocapture as per @efriedma / https://alive2.llvm.org/ce/z/EmvTq9.

Even if both of the unrollers are right as per their model
(LU duplicates whole loop body; while LU duplicates each instruction,
increasing live ranges, i believe), i'm mainly just worried
that two unroll strategies disagree in the end.

Could you please add this test: https://godbolt.org/z/WvYxnejnc (it should not optimize to unreachable/undef/poison)

LGTM, thank you.

RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-December/154249.html

Can the issues instead be viewed as missed transformations that should be implemented instead of trying to prevent creating those "bad" patterns?

Is there an exhaustive test for this method?

In D115179#3176871, @aeubanks wrote:

In D115179#3174484, @lebedev.ri wrote:

FTR i will be rather surprised if that alone would fully alleviate the need for this late loopdelete.

Changing the EarlyCSEPass in PassBuilder::buildFunctionSimplificationPipeline() to GVNPass does make llvm/test/Transforms/PhaseOrdering/deletion-of-loops-that-became-side-effect-free.ll pass if we delete the -O1 RUN line

Compile time is misleading.
What about run time impact?

And back to restrictive profitability check.

FTR i will be rather surprised if that alone would fully alleviate the need for this late loopdelete.

Relaxed the profitability check - i'm not sure this is better, and that is why it was there.

Forego of the undef constant folding, rebased.

ping

It would be good to mimic what the module equivalence check does
(when no changes were reported, the module should not have been changed),
and e.g. when in EXPENSIVE_CHECKS mode, if we weren't going to run those
extra vectorization passes because we predicted they wouldn't do anything,
to still run them, and check that they actually didn't do anything.

Ok, i guess i have a rather more general and radical, invasive solution, let's see if that works out first.

In D115016#3168935, @pengfei wrote:

which is not true for most recent cpus.

It seems Zen1's TP is still 2 in some cases. See Agner Fog's table and https://uops.info/table.html?search=mul%2064&cb_lat=on&cb_tp=on&cb_uops=on&cb_ports=on&cb_SKL=on&cb_ZENp=on&cb_ZEN2=on&cb_measurements=on&cb_doc=on&cb_base=on

Ack, this is the tragedy of these cost tables, they have to represent the worst-case, even if it is bogusly higher than the average.

In D114988#3168197, @nikic wrote:

I think you are missing the whole point there. It is explicitly NOT the point of this patch
to be able encode that some index must take values in range of [x, y). So if that is your proposal,
while it may be interesting, it's explicitly inferior, and does not solve the motivational case.

Right, sorry. I thought you wanted to solve the same issue discussed in the llvm-dev thread, but I see that you're more interested in SROA than AA here.

Yup.

In D114988#3167963, @nikic wrote:

While I support the general goal of exposing GEP offset restrictions to IR,
I am quite strongly opposed to the implementation approach of extending inrange.
The core issue is that this is strongly tied to LLVM struct types and structural GEP indexing.
This will be a blow to opaque pointer usefulness and future offset canonicalization for GEPs.

While i'm certainly sympathetic to the opaque pointer future,
i'd also like to remind that they are just a tool.

Hide the problem by defaulting TargetTransformInfoImplBase::useEmulatedMaskMemRefHack() to true.
If no triple is specifically requested, that is the TTI that is used.
This highlights that those tests are somewhat of a lie,
and raises questions about the implementation of those features.

Ok, so i'm looking at those optsize.ll/tripcount.ll tests, and i'm not sure what exactly they are testing.
They don't specify the triple/attributes, so what costs do they expect to get?

In D113520#3164812, @reames wrote:

@lebedev.ri Your suggested approach makes sense to me at a basic level. Forming the alloca is starting to seem more and more like a hack, but we can come back to implementing partial mem2reg as a follow on code improvement.

I think the obvious solution is:

1. Keep all stores. (in terms of this patch, iterate over all slices and duplicate all store instructions to also store into the cloned alloca)
2. Keep all loads (in terms of this patch, iterate over all slices and before the load from the original alloca, load from the cloned alloca and store into the original alloca) UNLESS we can omit particular load because we can tell that there was no taint (may-write calls) on every path from the every previous store.

LG

Not really familiar with PPC, but the TargetLowering.cpp part makes sense to me.

This is starting to sound like the isnan threads all over again. I don't envision making much progress with this approach...

That's a lot of complexity. It sounds like we are missing some cleanup transforms in some other passes?
Can you please post a standalone example of the "bad" codegen (via a godbolt link) that you are trying to avoid?

In D113520#3162304, @reames wrote:

Spent a decent amount of time looking at this, but haven't yet fully formed a recommendation. I'm going to summarize my findings to date in the hopes this is useful.

Thank you for taking a look!

LGTM, not sure if someone else should take a look.
Thank you.

In D113371#3161934, @RKSimon wrote:

In D113371#3161727, @lebedev.ri wrote:

Hm, i'm not sure i'm following, why is the truncation free if the dropped bits are zeros / why it won't be free otherwise?

Its not - IsTruncateFree is only true if we've extended both inputs, in which case truncating the inputs should be free, and we're better off doing that so we can perform the MULH on a smaller vector.

In D114766#3161737, @sepavloff wrote:

In D114766#3161688, @kpn wrote:

In D114766#3161680, @sepavloff wrote:

Moving this logic to wouldInstructionBeTriviallyDead() does not seem right, because the decision about removing side effect comes from constant evaluation and not from the state of call object.

I'm sympathetic to this argument. If a transformation pass determines that a constrained intrinsic can be removed then it would be helpful if that pass could note that fact. This doesn't help us with analysis passes, though. And I'm a little worried about having to explicitly mark constrained intrinsics as removable since I don't know that we do that for any other instruction. If constrained intrinsics are the only time we do this then it seems error prone. Maybe I'm wrong?

The ability to remove side effect is vital for the implementation of floating point arithmetic. There are several possible optimization techniques, which depend on this possibility. For example, if FP operations in a function have attributes "round.dynamic" and "fpexcept.ignore" and we know that rounding mode is not changed in this function, we could remove side effect of all the operations, which can give performance of non-constrained operations. As we cannot mix ordinary and constrained operation in the same function, the only way is to control side effect.

Hm, i'm not sure i'm following, why is the truncation free if the dropped bits are zeros / why it won't be free otherwise?

Now that the costmodel patch spam has winded down,
rebase&ping.

D111460 landed.

D111460 landed.

LG, thank you.

Tests?
Presumably this is default-off?

In D111460#3158819, @rengolin wrote:

I can't predict if the new shuffle patterns will be better or worse on each affected platform, least of all looking at IR, not ASM.

But if those changes have positive changes in code generation (translating to better benchmark numbers), then this looks good to me.

Rebased again.

In D114697#3158301, @RKSimon wrote:

LGTM - at least we know the code path is touched :)

In D114697#3157987, @RKSimon wrote:

How easy is it to add test coverage for this?

Rebased onto D114697.

Okay!
It took some time, but the costmodel for AVX512's masked interleaved has been completed.
I believe, this addressed all the review notes here.
Does anyone have any further closing remarks?
If not, i will be landing this shortly.

In D114316#3157873, @RKSimon wrote:

LGTM

ping

ping

In D114650#3157253, @nikic wrote:

To ask the obvious question: Can we get away with simply adding a getSCEV recursion limit instead? Is there reason to believe that a sensible recursion cutoff (similar to the many SCEV already has) would adversely affect analysis quality in practice? I'd rather avoid the additional complexity if we can.

In D114386#3157092, @spatel wrote:

In D114386#3155303, @lebedev.ri wrote:

LGTM, sorry for the delay, i like this.

Thanks for the suggestion and review! Let's see if this causes any fallout...

Thanks for looking into this!
I previously looked into this: https://github.com/LebedevRI/llvm-project/commit/6b006aa21caf018aa0f280828899d510274c8444
... but as it is apparent, never posted the diff. Nonetheless, it may be interesting to look at.

If SCCP runs again, will it then again create a new unreachable block
for the default that is already pointing at the unreachable block?
Should it not do such redundant work?

LGTM, sorry for the delay, i like this.

Thank you for looking into this.

Not really familiar with this pass, but the transform sounds legal, although i would have guessed you'd want to instead point it to an new block with unreachable?

In D114533#3151423, @arsenm wrote:

Patch description should include this avoids a need to introduce ptrtoint/inttoptr pairs

Rebased, NFC.
As far as i currently know, this is the last prerequisite for D114316.

Is there an RFC? This quite seems like asking for problems.

Poke, i think these two patches (this&D114316) are the last bits needed for D111460.