Since there doesn't seem to be any other suggestions, I see no reason why we should hold off on this landing.

LGTM.

LGTM.

D110256 has not been accepted yet, but is it OK to land this now (to be able to close PR51878)?

Can you add a codegen test for PR51878? The example listed in your message should do.

LGTM modulo minor typo that was missed from before.

LGTM.

LGTM.

LGTM modulo minor nit. Thanks!

As an alternative, we could update ImproveChain (and visitLIFETIME_END) to limit the aliasing around lifetime_start / end to disallow improving the chain dependence of a mem op node from a different lifetime node that may alias. That should prohibit access from two aliasable frame indices from being concurrent while still allowing us to leverage that disjoint allocs should be disjoint.

I've not touched this and probably will not for the forseeable future. Is someone else willing to take the lead on this?

I can't speak to the PPC issue, but from jyknight's comment:

LGTM. Thanks!

I havn't got the idea. Could you please elaborate what is the "SDNode *" in the key and what is the "SDNode *" in the value? And how it helps the early return in the dependence check?

It may not be easy for us to prune the searching space without missing some merging opportunity. I feel it is easier to achieve that goal by controlling how many times a {RootNode, StoreNode} pair appear in candidate set repeatedly. With the patch, no bit change in the binaries were found for clang and some large application. So the current patch is a safe change.

Ah. That's a tricky one. It sounds like this isn't a case of the candidate stores not being mergable, but rather us giving up before we completely verify that.

I check and the reason the candidates cannot be merged isn't because of the legal size. It is that checkMergeStoreCandidatesForDependencies always returns false so https://reviews.llvm.org/D60133 doesn't help.

This has come up a few times before (https://reviews.llvm.org/D60133 is the most recent instance). So far it's either been deemed not important enough or there was a way to limit the candidate search to limit the search to the correct size (e.g. checking the in context max valid store size and making sure there was a valid non-trivial store merge).

LGTM.

LGTM

It's reasonable assuming we want to prohibit the SystemZ cases. Note that TLI.canMergeConsecutiveStoresOfVectorElements and TLI.canMergeStoresTo are both only called here and should be merged into a single method.

Cuz PPC's allowsMemoryAccess lacks information about which CombineLevel is at, it fails the check(Only a few vector types are allowed to have misaligned access). As a result, currently no changes happen in PPC's code. I might try to solve this problem with another patch.

Looks like it's mostly an improvement though there are some potential regressions around vector shuffles. I'll leave it to the others if it's acceptable to land.

By double check, I just meant to look at the results again with isTypeLegal checked, which is where we are?

Hi, @niravd, after investigate code carefully, I think this check might not be redundant. Considering the case, we have 3 i32 values extracted from vectors, both isTypeLegal and TIL.isTypeLegal see a v3i32 illegal.

Should we have the corresponding subcarry case folded in here as well?

This seems like it should be folded into the already existing checks. Do you know why NumStoresToMerge was not being before. I expect it's the requirement of a legal types in pre-legal merges or PPC's check for allowed misaligned accesses missing some cases. If it's the former I suspect we can disable the legality requirement prelegaltypes for non-truncated stores (replace TLI.isTypeLegal(ty) with isTypeLegal(ty)).

Yes, I think that keeping the flag stuff to an atomic makes the most sense. We should probably make sure we're doing the right thing for the dereferenceable and invariant flags as well.

Don't we need to do something for non-temporal loads as well?

LGTM modulo typo nit. Thanks!

In D62633#1522420, @fhahn wrote:

In D62633#1522239, @niravd wrote:

Can you get away with just adding all remaining TFs (not their operands) to Ops? That way we keep the TF smaller and the pruning search still happens. That seems sufficient to fix the dropped operator issue.

Yes we can, kind of. If we just add all remaining TFs, we might get stuck in a loop, because in some cases the simplified TokenFactor == N and N gets added to the worklist, because it is the single user of another tokenfactor. I changed it to expand the first outstanding tokenfactor and add the remaining ones unexpanded. That way, we should not get stuck in a loop, while keeping the size small.

What do you think?

Can you get away with just adding all remaining TFs (not their operands) to Ops? That way we keep the TF smaller and the pruning search still happens. That seems sufficient to fix the dropped operator issue.

LGTM.

LGTM.

The current interfaces definitely do not provide anything like that. As I recall, most of the use cases requiring canMergeStoresTo involve ad hoc ways to locally declare a type/operation invalid localized to the specific target, so I suggest the just marginally expanding hte interface to capture the one issue we've seen so far (no-implict-float) and defer any deeper analysis for when we run into it.

This is okay modulo nits, but let's land the TokenFactor operand size limiting first as it also implicitly bounds things.

Notes inline, but I think the majority of the compile time improvements come from keepign TokenFactor operand counts bounded. This should be changed to do reflect that.

LGTM.

I think, make it unconditional.

I was thinking things pending from the folding optimization in getNode, but I missed that it was the target specific one so it should be fine. That said, I still think it's better to do it unconditionally so it'll be pruned by default with the future additio of Listener-based pruning if that proves too expensive.

I worry this is just fix just the dangling nodes as we see them. A brief glance at the AND construction gives us space for this to happen again. I'd much rather we do the removal check unconditionally or leverage something like
DAGUpdateListener to get a more fundamental measure of what nodes may be prunable.

DAGCombiner now has infrastructure to strip away nodes that were created but unused in during a visit. It probably makes sense to do something similar in ISel

LGTM.

It's not sufficient to check if you can merge two stores into a valid node; there are backends where you need 4 or more to get a legal merged store.

rL357283 supersedes this.

Consider inserted nodes for pruning.

Defer Worklist modification in NodeInserter to follow up patch (D58070).

Now that lifetime node hashing and TokenFactor cleanup has happened, factor out Dead Store improvements as they are no longer needed to prevent regressions.

Debug test also changes. h

Initial commit reverted due to Halide compile time explosion.

Address lingering test comments.

Landed in rL356996

Extract copied code into helper and format

Address comments.

Revert mips test changes