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

[SAHIL GIRISH YERAWAR via Phabricator via llvm-commits]

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