[PATCH] D63693: [Polly][NFC][ScopBuilder] Move buildAliasChecks to ScopBuilder

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

In D63693#1554880 <https://reviews.llvm.org/D63693#1554880>, @domada wrote:

> Please let me know if such big patch is acceptable. I have moved all functions referenced  only by `buildAliasChecks` function to ScopBuilder class.


Yes, if all changes have a common topic, in this case moving the alias check functionality. Even if we wanted tom move them, I don't think `hasFeasibleRuntimeContext` and `lookupBasePtrAccess` are specific to this functionality.

Could you change the title to reflect the topic, such as "Move buildAliasChecks and its implementing methods to ScopBuilder"?



================
Comment at: polly/include/polly/ScopBuilder.h:35-38
+  using AliasGroupTy = SmallVector<MemoryAccess *, 4>;
+
+  /// A vector of alias groups.
+  using AliasGroupVectorTy = SmallVector<AliasGroupTy, 4>;
----------------
AFICS these types are only used in private functions, i.e. could be private themselves.


================
Comment at: polly/include/polly/ScopInfo.h:2035
-  /// Return the access for the base ptr of @p MA if any.
-  MemoryAccess *lookupBasePtrAccess(MemoryAccess *MA);
-
----------------
I think this accessor might still be useful after havind constructed a SCoP on should stay.


================
Comment at: polly/include/polly/ScopInfo.h:2502
-  /// @returns True if the optimized SCoP can be executed.
-  bool hasFeasibleRuntimeContext() const;
-
----------------
This function might also be useful after the SCoP has been constructed and should stay.


================
Comment at: polly/include/polly/ScopInfo.h:2585
   /// Return all alias groups for this SCoP.
-  const MinMaxVectorPairVectorTy &getAliasGroups() const {
-    return MinMaxAliasGroups;
-  }
+  MinMaxVectorPairVectorTy &getAliasGroups() { return MinMaxAliasGroups; }
 
----------------
I don't think modifying an object's internals through a get-accessor is a good idea.


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:2381
   scop->simplifyContexts();
-  if (!scop->buildAliasChecks(AA)) {
+  if (!buildAliasChecks(AA, ORE)) {
     LLVM_DEBUG(dbgs() << "Bailing-out because could not build alias checks\n");
----------------
After being moved to ScopBuilder, `buildAliasChecks` can just use `this->AA`. Similarly, as a uitility object `ORE` can be made a member of `ScopBuilder` so it's not needed to be passed every time.


Repository:
  rPLO Polly

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63693/new/

https://reviews.llvm.org/D63693