LGTM

In D108699#3441724, @fhahn wrote:

Thanks for the update!

I think it would be good to split this up to have a first patch that just adds very limited support in findForkedSCEVs and then gradually add support for more cases separately. This also makes it easier to make sure all code paths in findForkedSCEVs are covered by the unit tests.

I went ahead and rebased D114487 and stripped the fork analysis to the bare minimum. I'd be happy to land the scaffolding in D114487 separately and this patch and following could add the sophisticated fork analysis.

Rebased. Ping?

Rebased, added a recursion limit to the SCEV building function.

Rebased and changed to use @fhahn 's lighter-weight approach from D114487 combined with my recursive function to find the SCEVs. Although the simplified tests are handled with a couple levels of checking, the real applications I was working on had additional operations between the ptr value for the load or store and a select.

Revised based on @fhahn 's initial suggestions, rebased.

Rebased, disabled by default, added a couple of different instructions into the tests to ensure those paths are at least covered even if they aren't in a positive case right now; I'm still planning to leave those cases (known strides, g/s, uniform) for a followup patch; I suppose I could start a patch series if people want to see those earlier.

Rebased, and changed the membership of a checking group to be a pair of PointerInfo index and fork so that printing can show which forks are present in a group.

Superseded by @lebedev.ri 's patches. I'll continue looking at the metadata angle separately.

Added the ability to look through GEPs for a call. I've limited this to using indexes of all 0, and only for single value types, so I'm not sure how often we'll encounter that. The limitation on the indexes does match the existing checks for whether promotion is allowed.

In D109749#3085994, @jdoerfert wrote:

In D109749#3078247, @huntergr wrote:

In D109749#3078176, @jdoerfert wrote:

Does this work for you:

diff --git a/llvm/lib/Analysis/CaptureTracking.cpp b/llvm/lib/Analysis/CaptureTracking.cpp
index 8955658cb9e7..41251d2676e6 100644
--- a/llvm/lib/Analysis/CaptureTracking.cpp
+++ b/llvm/lib/Analysis/CaptureTracking.cpp
@@ -373,9 +373,13 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
     case Instruction::Store:
       // Stored the pointer - conservatively assume it may be captured.
       // Volatile stores make the address observable.
-      if (U->getOperandNo() == 0 || cast<StoreInst>(I)->isVolatile())
+      if (U->getOperandNo() == 0 || cast<StoreInst>(I)->isVolatile()) {
+        if (auto *AI = dyn_cast<AllocaInst>(I->getOperand(1)->stripInBoundsOffsets()))
+          if (AI->hasMetadata("nocapture_storage"))
+            break;
         if (Tracker->captured(U))
           return;
+      }
       break;
     case Instruction::AtomicRMW: {
       // atomicrmw conceptually includes both a load and store from

And then add !nocapture_storage !0 after the alloca in your example as well as !0 = !{!0} in the end of that file

Ah, the 'nocapture_storage' metadata is what I've been missing, thanks. I'll update the diff once I've added that and adjusted the tests.

Technically, this is not yet something we have in the IR. We can reply to the old thread in which different solutions were discussed and
propose this one again. Then modify Clang to emit the metadata for the reduction case and land the diff I posted. All that said, it works
for your case, right?

Updated the diff based on the suggestion from @lebedev.ri

In D109749#3078046, @Meinersbur wrote:

In D109749#3077297, @huntergr wrote:

1. Although the address is loop invariant, the data isn't.

LICM does scalar promotion (controlled by -disable-licm-promotion), as in "promote memory location to register". It doesn't matter whether the value at the location is invariant. Whether this belongs into a pass called "Loop Invariant Code Motion" is a different question.

2. For this loop in particular, the store is conditional so might never happen. We *could* add a second boolean reduction to determine whether or not to actually perform a store after the loop, but that's a bit more complicated than just letting mem2reg do what it should.

This patch adds another pass, not make mem2reg do it. LICM currently does not handle conditional control flow for scalar promotion, but it should require much less code to change that. See the use of isGuaranteedToExecute in llvm::promoteLoopAccessesToScalars.

Rebased, minor fixes from review comments.

In D109749#2999389, @lebedev.ri wrote:

I have seen cases where this would be beneficial,
some of those are just due to lack of inlining, but not all.

I strongly believe this should be part of SROA,
it should analyze the alloca's ignoring captures,
and if it is otherwise promoteable, it should:

1. duplicate the original alloca (only for simplicity, this is fine since we know the old alloca goes away)
2. before each capture, load contents of the old alloca, and store it into new alloca
3. after each capture, load contents of the new alloca, and store it into old alloca
4. change captures to refer to the new alloca
5. run AggLoadStoreRewriter on the new alloca - so that all the uses of old alloca we've just introduced are analyzeable by SROA
6. proceed with normal handling of the old alloca - mem2reg will now succeed

Ping.

Rebased, updated based on review comments.

Rebased, ran clang-format over the patch.

I'm fixing up the buildbot failure - other parts of the repo (e.g. openmp) fail to build with -Werror so I don't normally use it.

Ping.

Apologies for not including more of the diff for context, but the test files are large enough that the full diff exceeds the maximum upload size.

Looks good, I agree with @dmgreen that a clang driver test would be nice.

• Removed the ) my editor helpfully added to the CHECK line
• Added a test to declare_simd_aarch64.c

• Added a function body to the test so that it would generate symbols
• Added check lines to ensure the mangled name is present
• Reformatted the params of Sema::CheckOpenMPLinearDecl to comply with coding style.

Superseded by D66302

Superseded by D65448

Superseded by D68203

• Removed const qualifier from queries
• Swapped to std::make_tuple for the comparisons in CodeGenDAGPatterns.cpp

I also changed the SelectionDAGBuilder code for masked loads/stores/gathers/scatters to use the known min size when creating a MachineMemoryOperand, since MMO isn't aware of scalable types yet. I've left TODOs as reminders.

• Switched the lambdas in CodeGenDAGPatterns to use std::tie for ordering. This needed the TypeSize objects to be constant (I get a "non-const lvalue reference" error otherwise), so I changed all query methods to return const objects.
• Changed stack offset to deliberately use a TypeSize. This hit another problem with implicit promotion and signed v. unsigned casting, so I've added explicit casts.

In D68476#1725466, @mgudim wrote:

Renamed the inrinsics according to the convention.

• Refactored to match the same approach used in D53137 -- TypeSize returned for all size queries instead of introducing separate interfaces and relying on implicit conversion to be (mostly) compatible with existing code.
• Added 'isByteSized()' interface to TypeSize.
• Minor bugfix to DAGCombiner::ForwardStoreValueToDirectLoad, where implicit conversions allowed an unsigned number to wrap and then be converted to a larger signed integer; with TypeSize using uint64_t instead, this showed up during a make check-all run with ubsan active. Can't write a test for that one since the transform bailed out when Offset was non-zero.

I was also expecting something that says "setOperationAction(ISD::MLOAD, VT, Legal)" somewhere, but I guess that's already the default?

Hi, thanks for waiting a bit. LGTM.

• Fixed legalization action code, which was trying to use widening for all integer scalable vector types after the new MVT ranges were added
• Added unit tests for legalizing the result via promotion
• Minor cleanups, additional comments as requested

Ping.

Hmm, forgot to add the last round of minor fixes before committing. Sorry about that, will push them as well.

• Added DAG combine to canonicalize a build_vector splat into a splat_vector if the target supports the splat_vector operation (by overriding the default Expand legalization action)

Hi James,

• Renamed ScalableSize to TypeSize, including header name.
• added alignTo function that takes and returns a TypeSize. I wasn't sure if this should be added to MathExtras.h where the other variants live, so just kept it in TypeSize.h for now

• Removed intrinsic and supporting code
• Added new ISD as discussed on llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2019-August/134809.html

You can add the extra multiclass for ftsmul if you wish, but it's not needed until someone implements a matching pattern for that instruction. We only match it against an ACLE intrinsic downstream, not common SDag nodes.

Thanks for taking an interest in SVE codegen. The tests look fine, but we had a different approach in mind for generating many of the patterns. In this case instead of making separate def : Pat< lines for each type, we add an SDPatternOperator and pattern to the multiclass for that instruction type.

• Changed existing interface to return ScalableSize objects and added a (mostly) transparent conversion, as per Sander's suggestion.
• Removed new interfaces for DataLayout and Type.
• Fixed cases where the transparent conversion doesn't quite work (e.g. std::max/min, where the types must be the same)

In D66871#1649148, @cameron.mcinally wrote:

LGTM

Does anyone like Sander's suggestion to make ScalableSize (or whatever we end up naming it) the return value for all size queries and provide an overloaded cast operator to transparently work with existing code comparing against unsigned values? Or is it preferable to keep the current split?

• Split out backend code into separate patches
• Renamed 'Min' to 'KnownMin' in method names.
• Added a few more comments.

• Reordered MVTs to group fixed-length types together (and the same for scalable).
• Replaced existing range accessors to remove explicit isScalableVector checks on MVTs in various backends.

Split out from D53137.

Created D66339 to split out the code to skip scalable vector types in other backends.

In D53137#1621351, @sdesmalen wrote:

Thanks @huntergr for working on this!

This patch can probably be split into two separate patches, which make them easier to review;

• One that fixes the Targets to ignore scalable types (see comment)
• Another one that adds the interface for scalable size queries.

• Added comments explaining the new methods
• Added tests for the MVT/EVT/DataLayout interfaces
• Refactored ScalableSize class operators to build on '==' and '<'
• Added checks in various backends that don't support scalable vectors to prevent legalizing operations involving scalable MVTs

• Added inline comments to clarify boolean parameter meaning.