[PATCH] D45066: [Polly] [ScopInfo] Remove bail out condition in buildMinMaxAccess()

SAHIL GIRISH YERAWAR via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 16 01:21:48 PDT 2018


cs15btech11044 added inline comments.


================
Comment at: lib/Analysis/ScopInfo.cpp:2303
 /// Add the minimal/maximal access in @p Set to @p User.
-static isl::stat
-buildMinMaxAccess(isl::set Set, Scop::MinMaxVectorTy &MinMaxAccesses, Scop &S) {
+static isl::stat buildMinMaxAccess(isl::set &Set,
+                                   Scop::MinMaxVectorTy &MinMaxAccesses,
----------------
Meinersbur wrote:
> Is there a reason to change `Set` to by-reference?
As mentioned in another comment, I had some confusion before regarding function arguments as references and as actual values.  It's all cleared up now.

I think that this change is unnecessary. Changes will be reflected in the next patch.


================
Comment at: lib/Analysis/ScopInfo.cpp:2317-2318
 
   if (isl_set_n_basic_set(Set.get()) >= MaxDisjunctsInDomain)
     return isl::stat::error;
 
----------------
Meinersbur wrote:
> IMHO this condition can be removed.
Okay, I agree with this point.

Changes will be reflected in the next patch.


================
Comment at: lib/Analysis/ScopInfo.cpp:2393
   auto Lambda = [&MinMaxAccesses, &S](isl::set Set) -> isl::stat {
-    return buildMinMaxAccess(Set, MinMaxAccesses, S);
+    isl::set &SimpleSet = Set;
+    return buildMinMaxAccess(SimpleSet, MinMaxAccesses, S);
----------------
Meinersbur wrote:
> Why introducing an alias for `Set`?
I had some confusion before regarding function arguments as references and as actual values. It's all cleared up now.

I agree that this introduction of an alias is unnecessary. Changes will be reflected in the next patch.


https://reviews.llvm.org/D45066





More information about the llvm-commits mailing list