[llvm] r290734 - [LICM] Make logic in promoteLoopAccessesToScalars easier to follow. NFC.

[Philip Reames via llvm-commits]

Very nice cleanup.  Thank you.

Philip

On 12/29/2016 04:39 PM, Michael Kuperstein via llvm-commits wrote:
> Author: mkuper
> Date: Thu Dec 29 18:39:00 2016
> New Revision: 290734
>
> URL: http://llvm.org/viewvc/llvm-project?rev=290734&view=rev
> Log:
> [LICM] Make logic in promoteLoopAccessesToScalars easier to follow. NFC.
>
> Modified:
>      llvm/trunk/lib/Transforms/Scalar/LICM.cpp
>
> Modified: llvm/trunk/lib/Transforms/Scalar/LICM.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LICM.cpp?rev=290734&r1=290733&r2=290734&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/LICM.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/LICM.cpp Thu Dec 29 18:39:00 2016
> @@ -904,23 +904,24 @@ bool llvm::promoteLoopAccessesToScalars(
>     // is not safe, because *P may only be valid to access if 'c' is true.
>     //
>     // The safety property divides into two parts:
> -  // 1) The memory may not be dereferenceable on entry to the loop.  In this
> +  // p1) The memory may not be dereferenceable on entry to the loop.  In this
>     //    case, we can't insert the required load in the preheader.
> -  // 2) The memory model does not allow us to insert a store along any dynamic
> +  // p2) The memory model does not allow us to insert a store along any dynamic
>     //    path which did not originally have one.
>     //
> -  // It is safe to promote P if all uses are direct load/stores and if at
> -  // least one is guaranteed to be executed.
> -  bool GuaranteedToExecute = false;
> -
> -  // It is also safe to promote P if we can prove that speculating a load into
> -  // the preheader is safe (i.e. proving dereferenceability on all
> -  // paths through the loop), and that the memory can be proven thread local
> -  // (so that the memory model requirement doesn't apply.)  We first establish
> -  // the former, and then run a capture analysis below to establish the later.
> -  // We can use any access within the alias set to prove dereferenceability
> +  // If at least one store is guaranteed to execute, both properties are
> +  // satisfied, and promotion is legal.
> +  // This, however, is not a necessary condition. Even if no store/load is
> +  // guaranteed to execute, we can still establish these properties:
> +  // (p1) by proving that hoisting the load into the preheader is
> +  // safe (i.e. proving dereferenceability on all paths through the loop). We
> +  // can use any access within the alias set to prove dereferenceability,
>     // since they're all must alias.
> -  bool CanSpeculateLoad = false;
> +  // (p2) by proving the memory is thread-local, so the memory model
> +  // requirement does not apply, and stores are safe to insert.
> +
> +  bool DereferenceableInPH = false;
> +  bool SafeToInsertStore = false;
>   
>     SmallVector<Instruction *, 64> LoopUses;
>     SmallPtrSet<Value *, 4> PointerMustAliases;
> @@ -969,8 +970,8 @@ bool llvm::promoteLoopAccessesToScalars(
>           if (!Load->isSimple())
>             return Changed;
>   
> -        if (!GuaranteedToExecute && !CanSpeculateLoad)
> -          CanSpeculateLoad = isSafeToExecuteUnconditionally(
> +        if (!DereferenceableInPH)
> +          DereferenceableInPH = isSafeToExecuteUnconditionally(
>                 *Load, DT, CurLoop, SafetyInfo, Preheader->getTerminator());
>         } else if (const StoreInst *Store = dyn_cast<StoreInst>(UI)) {
>           // Stores *of* the pointer are not interesting, only stores *to* the
> @@ -981,28 +982,29 @@ bool llvm::promoteLoopAccessesToScalars(
>           if (!Store->isSimple())
>             return Changed;
>   
> -        // Note that we only check GuaranteedToExecute inside the store case
> -        // so that we do not introduce stores where they did not exist before
> -        // (which would break the LLVM concurrency model).
> -
> -        // If the alignment of this instruction allows us to specify a more
> -        // restrictive (and performant) alignment and if we are sure this
> -        // instruction will be executed, update the alignment.
> -        // Larger is better, with the exception of 0 being the best alignment.
> +        // If the store is guaranteed to execute, both properties are satisfied.
> +        // We may want to check if a store is guaranteed to execute even if we
> +        // already know that promotion is safe, since it may have higher
> +        // alignment than any other guaranteed stores, in which case we can
> +        // raise the alignment on the promoted store.
>           unsigned InstAlignment = Store->getAlignment();
> -        if ((InstAlignment > Alignment || InstAlignment == 0) &&
> -            Alignment != 0) {
> +        if (!InstAlignment)
> +          InstAlignment =
> +              MDL.getABITypeAlignment(Store->getValueOperand()->getType());
> +
> +        if (!DereferenceableInPH || !SafeToInsertStore ||
> +            (InstAlignment > Alignment)) {
>             if (isGuaranteedToExecute(*UI, DT, CurLoop, SafetyInfo)) {
> -            GuaranteedToExecute = true;
> -            Alignment = InstAlignment;
> +            DereferenceableInPH = true;
> +            SafeToInsertStore = true;
> +            Alignment = std::max(Alignment, InstAlignment);
>             }
> -        } else if (!GuaranteedToExecute) {
> -          GuaranteedToExecute =
> -              isGuaranteedToExecute(*UI, DT, CurLoop, SafetyInfo);
>           }
>   
> -        if (!GuaranteedToExecute && !CanSpeculateLoad) {
> -          CanSpeculateLoad = isDereferenceableAndAlignedPointer(
> +        // If the store is not guaranteed to execute, we may still get
> +        // deref info through it.
> +        if (!DereferenceableInPH) {
> +          DereferenceableInPH = isDereferenceableAndAlignedPointer(
>                 Store->getPointerOperand(), Store->getAlignment(), MDL,
>                 Preheader->getTerminator(), DT);
>           }
> @@ -1021,18 +1023,23 @@ bool llvm::promoteLoopAccessesToScalars(
>       }
>     }
>   
> -  // Check legality per comment above. Otherwise, we can't promote.
> -  bool PromotionIsLegal = GuaranteedToExecute;
> -  if (!PromotionIsLegal && CanSpeculateLoad) {
> -    // If this is a thread local location, then we can insert stores along
> -    // paths which originally didn't have them without violating the memory
> -    // model.
> +
> +  // If we couldn't prove we can hoist the load, bail.
> +  if (!DereferenceableInPH)
> +    return Changed;
> +
> +  // We know we can hoist the load, but don't have a guaranteed store.
> +  // Check whether the location is thread-local. If it is, then we can insert
> +  // stores along paths which originally didn't have them without violating the
> +  // memory model.
> +  if (!SafeToInsertStore) {
>       Value *Object = GetUnderlyingObject(SomePtr, MDL);
> -    PromotionIsLegal =
> +    SafeToInsertStore =
>           isAllocLikeFn(Object, TLI) && !PointerMayBeCaptured(Object, true, true);
>     }
>   
> -  if (!PromotionIsLegal)
> +  // If we've still failed to prove we can sink the store, give up.
> +  if (!SafeToInsertStore)
>       return Changed;
>   
>     // Otherwise, this is safe to promote, lets do it!
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits