[PATCH] Loop Versioning for LICM

[Nema, Ashutosh]

Thanks Hal for looking into this.

> Like the vectorizer, you'll need some metadata that you can add to the loop 
> so that, when this pass is run multiple times over the same code, it does not 
> recursively multi-version the same loop.
Sure I'll add it.

> This pass needs some nice descriptive text near the top. 
> The summary you wrote for this review would make a 
> good start (added as a comment near the top of the file).
Will update it.

> ================
> Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:253
> @@ +252,3 @@
> +    MPM.add(createLoopVersioningPass());        // Do Loop Versioning
> +    MPM.add(createLICMPass());                  // Hoist loop invariants
> +  }
> ----------------
> Why are you running LICM here? I thought that the versioning pass had an 
> internal LICM? Or is this to hoist the checks?
There are couple of reasons for running this pass here.
First it will help us to hoist checks. In some cases observed scalar evolution 
was not updated and it's not having invariant information for versioned loop.
Running this later to ensure, hoisting of invariants.

> ================
> Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:466
> @@ +465,3 @@
> +  // 3) Check store instruction execution guarantee.
> +  // 4) Loop body should have a invariant store.
> +  // 5) Loop body should not have any may throw instruction.
> ----------------
> What is special about having an invariant store?
> 
> If we're interested in hoisting loop invariant loads that we cannot prove are 
> loop invariant, we can still have a problem if a loop-varying store aliasing with 
> the load in any iteration, no?
Invariant store check is for 'promoteLoopAccessesToScalars'. 
As in this implementation we only calling 'promoteLoopAccessesToScalars'.
But yes, we should consider invariant loads as well.
I'll update my changes and come back.

> ================
> Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:476
> @@ +475,3 @@
> +      CallInst *CI = dyn_cast<CallInst>(it);
> +      if (CI && !isa<DbgInfoIntrinsic>(CI)) {
> +        DEBUG(dbgs() << "    loop body has a call instruction\n");
> ----------------
> This is a bit overkill. You should certainly ignore readnone calls. 
> Why not reuse the logic here currently used by the vectorizer to 
> decide which calls are safe?
Sure, will update changes accordingly.

> ================
> Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:518
> @@ +517,3 @@
> +    } // Next instr.
> +  }   // Next block.
> +  LAI = &LAA->getInfo(CurLoop, Strides);
> ----------------
> If you feel the need to add these kinds of comments on the closing curly brace, 
> please split out a helper function instead.
Yes, that block looks little big, I'll try to split to a helper function.

> ================
> Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:811
> @@ +810,3 @@
> +    // Create alias scope domain.
> +    MDBuilder MDB(I->getContext());
> +    MDNode *NewDomain = MDB.createAnonymousAliasScopeDomain("LVDomain");
> ----------------
> This metadata-adding code should be a separate function.
Sure will move this to a helper function.

> ================
> Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:831
> @@ +830,3 @@
> +        NoAliases.push_back(NewScope);
> +        Scopes.push_back(NewScope);
> +        // Set no-alias for current instruction.
> ----------------
> I don't think this is right because you'll mark as noalias access to the same 
> underlying object that might, in fact, alias. We'll need to change this slightly, 
> and this will become easier, once we have the upcoming llvm.noalias intrinsic.
I didn’t understood this point completely. Are you pointing about 'NewScope' object ?
This object getting created by 'NewDomain' and 'Name'. these remains same so 
Every time we have the same object. We should have different object everytime.

Thanks,
Ashutosh

-----Original Message-----
From: hfinkel at anl.gov [mailto:hfinkel at anl.gov] 
Sent: Tuesday, May 19, 2015 1:46 AM
To: Nema, Ashutosh; listmail at philipreames.com; anemet at apple.com; hfinkel at anl.gov
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Loop Versioning for LICM

First, thanks for working on this. I apologize for the delay in looking at it.

Some high-level comments first...

Like the vectorizer, you'll need some metadata that you can add to the loop so that, when this pass is run multiple times over the same code, it does not recursively multi-version the same loop.

This pass needs some nice descriptive text near the top. The summary you wrote for this review would make a good start (added as a comment near the top of the file).


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:253
@@ +252,3 @@
+    MPM.add(createLoopVersioningPass());        // Do Loop Versioning
+    MPM.add(createLICMPass());                  // Hoist loop invariants
+  }
----------------
Why are you running LICM here? I thought that the versioning pass had an internal LICM? Or is this to hoist the checks?


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:466
@@ +465,3 @@
+  // 3) Check store instruction execution guarantee.
+  // 4) Loop body should have a invariant store.
+  // 5) Loop body should not have any may throw instruction.
----------------
What is special about having an invariant store?

If we're interested in hoisting loop invariant loads that we cannot prove are loop invariant, we can still have a problem if a loop-varying store aliasing with the load in any iteration, no?


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:476
@@ +475,3 @@
+      CallInst *CI = dyn_cast<CallInst>(it);
+      if (CI && !isa<DbgInfoIntrinsic>(CI)) {
+        DEBUG(dbgs() << "    loop body has a call instruction\n");
----------------
This is a bit overkill. You should certainly ignore readnone calls. Why not reuse the logic here currently used by the vectorizer to decide which calls are safe?


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:518
@@ +517,3 @@
+    } // Next instr.
+  }   // Next block.
+  LAI = &LAA->getInfo(CurLoop, Strides);
----------------
If you feel the need to add these kinds of comments on the closing curly brace, please split out a helper function instead.


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:811
@@ +810,3 @@
+    // Create alias scope domain.
+    MDBuilder MDB(I->getContext());
+    MDNode *NewDomain = MDB.createAnonymousAliasScopeDomain("LVDomain");
----------------
This metadata-adding code should be a separate function.


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:831
@@ +830,3 @@
+        NoAliases.push_back(NewScope);
+        Scopes.push_back(NewScope);
+        // Set no-alias for current instruction.
----------------
I don't think this is right because you'll mark as noalias access to the same underlying object that might, in fact, alias. We'll need to change this slightly, and this will become easier, once we have the upcoming llvm.noalias intrinsic.

http://reviews.llvm.org/D9151

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/