This is an archive of the discontinued LLVM Phabricator instance.

[LoopIdiom] Recognize unordered atomic memset
AcceptedPublic

Authored by dneilson on Nov 29 2017, 10:24 AM.

Details

Summary

This expands the loop idiom recognition pass to recognize the unordered
atomic memset pattern. This pattern is the same pattern as a regular memset
except that the stores are unordered atomic, and limited to register size.

Event Timeline

dneilson created this revision.Nov 29 2017, 10:24 AM
anna added inline comments.Dec 1 2017, 7:05 AM
lib/IR/IRBuilder.cpp
106

Given there's some constraints for unordered memset, does it make sense to add the constraints as asserts here (ptrAlign relation to ElementSize)?

Any other constraints?

lib/Transforms/Scalar/LoopIdiomRecognize.cpp
943

Stylistic comment: Could you switch around the if else so that smaller body is first?

test/Transforms/LoopIdiom/unordered-atomic-memset-noarch.ll
6

where's this max element size of 0?

dneilson added inline comments.Dec 1 2017, 7:40 AM
lib/IR/IRBuilder.cpp
106

Easy enough. Good point.

test/Transforms/LoopIdiom/unordered-atomic-memset-noarch.ll
6

I should clarify this better in the test comment... This is a noarch test, so a call to TTI->getAtomicMemIntrinsicMaxElementSize() will return 0, thus preventing the transform.

dneilson updated this revision to Diff 125145.Dec 1 2017, 7:58 AM
  • Addressing review comments.
anna added inline comments.Dec 1 2017, 11:21 AM
lib/Transforms/Scalar/LoopIdiomRecognize.cpp
923

Why do we not check that the "ordinary memset" was substituting a store?
In other words, shouldn't there be a check for SI

This is the behaviour before your change as well (we dont check for store), but somehow it struck me as weird.

dneilson added inline comments.Dec 1 2017, 11:25 AM
lib/Transforms/Scalar/LoopIdiomRecognize.cpp
923

processLoopStridedStore() kind of has two modes of operation.

  1. A loop of stores being converted into a single memset call
  2. A loop of memset calls being merged into a single memset call

I made the atomic case explicitly check for a store instruction here because the "loop of atomic memsets" hasn't been implemented yet.

anna accepted this revision.Dec 1 2017, 11:50 AM

LGTM

lib/Transforms/Scalar/LoopIdiomRecognize.cpp
923

makes sense. thanks for the clarification.

This revision is now accepted and ready to land.Dec 1 2017, 11:50 AM
dneilson updated this revision to Diff 149226.May 30 2018, 6:42 PM
  • Rebaseline, just to keep this up-to-date.