[PATCH] Hoist load based on alias analysis

[Gerolf Hoflehner]

Please see below. I still think the compile-time concern needs more attention. Ignoring that the patch LGTM. 

On Oct 28, 2014, at 12:37 AM, Elena Demikhovsky <elena.demikhovsky at intel.com> wrote:

>> Does the new implementation catch any cases the old one didn't? There is extra cost by walking the range of instruction twice, and perhaps a small change would have the same effect.
> [Demikhovsky, Elena] Exactly, not every store is a barrier for load hoisting. I check this with AliasAnalysis. I attached a test case where it works.

Ok, thanks.
> 
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:265
> @@ +264,3 @@
> +    BasicBlock *BB0 = Load0->getParent();
> +    if (Load0->IsIndenticalToWhenDefefined(Load1) &&
> +        !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1) &&
> ----------------
> Gerolf wrote:
>> Typo. Did you mean IsIdenticalToDefined()? Or IsIdendicalLoad()?
> IsIdenticalToDefined is a standard function to compare 2 instructions.
> 
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:267
> @@ +266,3 @@
> +        !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1) &&
> +        !isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0)) {
> +      return Load1;
> ----------------
> Gerolf wrote:
>> Are you checking the range for load0 twice? See overall comment.
> No. I check only once for each load. I can't say that instruction is a barrier before I see the load itself. Because I go back from bb start to the current instruction, I do it after I see two identical loads.

There have been concerns aboutcompile-time for the original. Here I still think you double your search space on both sides of the diamond. First, you look or load0, then you check if there is a constraint for load0. Then you look for load1. Then you check if there is a constraint for load1. At least you don’t have separate search and check on the “right” side since you already know which load you are looking for.

> 
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:371
> @@ -393,1 +370,3 @@
> +    LoadInst *L0 = dyn_cast<LoadInst>(I);
> +    if (!L0 || !L0->isSimple() || L0->isUsedOutsideOfBlock(Succ0))
>       continue;
> ----------------
> Gerolf wrote:
>> The optimization should work when one load is used outside the block. The optimization can work when both loads are used outside but we have to deal with the PHI nodes accordingly. 
> I did not change the current behavior in this case, but I plan to work more on these optimizations. Conditional load/store are barriers for vectorizer.
> 
> http://reviews.llvm.org/D5991
> 
>