This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] MergeConsecutiveStores - improve non-temporal load\store handling (PR42123)
ClosedPublic

Authored by RKSimon on Jun 5 2019, 8:15 AM.

Details

Summary

This patch is the first step towards ensuring MergeConsecutiveStores correctly handles non-temporal stores:

1 - When merging load\stores we must ensure that they all have the same non-temporal flag. This is unlikely to occur, but can in strange cases where we're storing at the end of one page and the beginning of another.

2 - The merged load\store node must retain the non-temporal flag.

After this I need to improve alignment handling - in most cases we shouldn't merge nt-ops unless they are naturally aligned - either we just prevent it in all cases or we refactor canMergeStoresTo\allowsMemoryAccess to include access to the alignment and MMO flags as well?

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Jun 5 2019, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 8:15 AM
niravd added a comment.Jun 5 2019, 8:37 AM

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

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

Yes we will - do you want that as part of this patch?

niravd added a comment.Jun 5 2019, 8:53 AM

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.

RKSimon updated this revision to Diff 203205.Jun 5 2019, 10:53 AM
RKSimon retitled this revision from [DAGCombine] MergeConsecutiveStores - improve non-temporal store handling (PR42123) to [DAGCombine] MergeConsecutiveStores - improve non-temporal load\store handling (PR42123).
RKSimon edited the summary of this revision. (Show Details)

Add nt-load handling as well

niravd added inline comments.Jun 5 2019, 11:36 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
15600 ↗(On Diff #203205)

Looks like we were erroneously transferring dereferencable from load to store on merge before. I think we need to do a store version of the isDereferencable calculation or we'll only preserve it on truncstores.

RKSimon marked an inline comment as done.Jun 5 2019, 11:50 AM
RKSimon added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
15600 ↗(On Diff #203205)

Were we? The getStore() didn't use any MMO flags before.

niravd accepted this revision.Jun 5 2019, 12:11 PM
niravd added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
15600 ↗(On Diff #203205)

Oh right! We're still being a bit weird about Dereferencable handling and that seems worth more of a look, but if we're being consistent with the past, there's no sense in delaying this patch.

This revision is now accepted and ready to land.Jun 5 2019, 12:11 PM

Thanks @niravd - any thoughts on how to tackle under-aligned nt-ops? My gut feeling is that just bailing on the merge if they aren't naturally aligned would be fine - do you know of any nt vector instructions that can handle non-aligned pointers?

This revision was automatically updated to reflect the committed changes.