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

[Ariel Ben-Yehuda via llvm-commits]

I think the performance worry is already a problem with my previous
patch - we already call isValidAssumeForContext quite a bit of times
in ValueTracking, where it will perform the long walk on a big BB.

On Sun, Sep 3, 2017 at 6:58 PM, Hal Finkel via Phabricator
<reviews at reviews.llvm.org> wrote:
> 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
>
>
>