[PATCH] Review for hoisting and sinking of equivalent memory instruction (Instruction Merge Pass)

[Gerolf Hoflehner]

Please see below.

On Jul 14, 2014, at 10:43 AM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:

> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:72
> @@ +71,3 @@
> +// b) Phabricator: http://reviews.llvm.org/D4096
> +//
> +//===----------------------- TODO -----------------------------------------===//
> ----------------
> I don't think we should reference this in the file.
Ok, looks like no reviewer likes the refs. Removed.
> 
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:266
> @@ +265,3 @@
> +      break;
> +    if (isa<LoadInst>(Inst)) {
> +      if (Inst->isUsedOutsideOfBlock(Inst->getParent()))
> ----------------
> Less nested control flow (also makes the logic easier to read):
> 
>  if (!isa<LoadInst>(Inst))
>    continue;
>  if (Inst->isUsedOutsideOfBlock(Inst->getParent()))
>    continue;
Ok. Done. It would be nice to have the coding standard integrated into the FE and get an error/coding standard warning. But this would probably be hard to push into an existing project. 
> 
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:324
> @@ +323,3 @@
> +      (A0->getParent() == L0->getParent()) && A1->hasOneUse() &&
> +      (A1->getParent() == L1->getParent()) && isa<GetElementPtrInst>(A0)) {
> +    DEBUG(dbgs() << "Hoist Instruction into BB \n"; BB->dump();
> ----------------
> Are we guaranteed that the GEP is recursively movable (are the GEP's inputs defined in BB or in AO->getParent())?
> 
> Say we had:
> 
>  %g0 = gep ...
>  %g1 = gep %g0
>  %l = load %g1
> 
> Would this still work?

Brilliant catch! Yes, I made an implicit assumption that your case cannot exist and did not make it explicit eg. by an assertion. More than happy to fix!

> 
> 
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:359
> @@ +358,3 @@
> +      LoadInst *L0 = dyn_cast<LoadInst>(I);
> +      if (L0->isSimple()) {
> +        ++NLoads;
> ----------------
> You can rewrite this with less nested control flow as:
> 
>  auto LI = dyn_cast<LoadInst>(I);
>  if (!LI || !LI->isSimple())
>    continue;

done.
> 
> ================
> Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:525
> @@ +524,3 @@
> +    // Sink move non-simple (atomic, volatile) stores
> +    if (isa<StoreInst>(I)) {
> +      StoreInst *S0 = dyn_cast<StoreInst>(I);
> ----------------
> You can rewrite this with less control flow see above.
done.
> 
> http://reviews.llvm.org/D4096
> 
>