[PATCH] Hoist load based on alias analysis

[Demikhovsky, Elena]

Thank you. Committed revision 221091.

> I still think the compile-time concern needs more attention.
I do this check only after I see 2 identical loads. The alternative is checking barriers for every Load0 before I know that I have a candidate for hoisting in the second block.

Btw, SimplifyCFG() does half way by hoisting identical instructions from diamond, but we do  load hoisting in a separate pass. Here I have a compile-time concern.
The same about store sinking.

-  Elena

-----Original Message-----
From: Gerolf Hoflehner [mailto:ghoflehner at apple.com] 
Sent: Thursday, October 30, 2014 23:10
To: reviews+D5991+public+da2d85a842ba3464 at reviews.llvm.org
Cc: Demikhovsky, Elena; hfinkel at anl.gov; llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Hoist load based on alias analysis

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
> 
> 

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.