[PATCH] D37216: [SROA] propagate !range metadata when moving loads

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

This looks good, but, have you checked for compile-time regressions? I'm concerned here because:

We've now made addAssumptionsFromMetadata linear in the number of instructions between the instruction and its replacement. That should be documented with the function declaration (at least). As the replacement always dominates the original instruction, when they're in the same BB, we'll always be doing that linear walk. As you're making the walk go through normal loads, I can imagine this becoming quadratic in some inputs (unless there's something about the way that SROA uses this that prevents that, and if so, we need to document that too).

If this can become quadratic, I think we'll want to maintain some kind of BB-region analysis, there we partition each BB into regions based on guaranteed execution, and then use that instead of walking.



================
Comment at: include/llvm/Analysis/ValueTracking.h:375
+  /// Return true if, when CxtI executes, it is guaranteed that either
+  /// I had executed already or that I is guaranteed to be reached and
+  /// be executed.
----------------
reached and executed -> later executed


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:314
+				       AssumptionCache *AC)
+{
+  if (LI->getMetadata(LLVMContext::MD_nonnull) &&
----------------
Brace on previous line.


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:316
+  if (LI->getMetadata(LLVMContext::MD_nonnull) &&
+      !llvm::isKnownNonNullAt(ReplVal, LI, &DT)) {
+    addAssumeNonNull(AC, LI);
----------------
Do you need the llvm:: here? If not, please remove it.


https://reviews.llvm.org/D37216