Page MenuHomePhabricator

[ScopBuilder] Move buildContext function from ScopInfo
Needs ReviewPublic

Authored by domada on May 22 2019, 11:37 AM.



Move buildContext function from ScopInfo to ScopBuilder.

Diff Detail

Event Timeline

domada created this revision.May 22 2019, 11:37 AM
Herald added a project: Restricted Project. · View Herald Transcript

Thank you for your contribution. A few remarks for your first upload for Polly on Phabricator:

  • Since these patches are mailed to llvm-commits, we were asked to add [Polly] into the title to make it immediately obvious that it's a patch for a subproject. The tag would be removed for the commit again.
  • Append "NFC" (No Function Change intended) to the title for a refactoring patch. This one stays for the commit message.
  • Add polly-dev (and llvm-commits) to subscribers.
  • Add grosser to the reviewer list. He's code owner for Polly.

I am split with this patch. I generally think that we should avoid calling methods in constructors because it might be surprising which method is actually called. IMHO constructors are for initializing fields but that's exactly what buildContext does. Unfortunately, moving it to ScopBuilder means it directly accesses a foreign object's private fields. That's arguably worse. At some point we would like to drop the friend class ScopBuilder in ScopInfo such that such accesses are not possible anymore.

Methods that I think are better suited to be moved from ScopInfo to ScopBuilder:

  • buildInvariantEquivalenceClasses
  • buildDomains
  • addUserAssumptions
  • buildSchedule
  • finalizeAccesses
  • addUserContext
  • addRecordedAssumptions
  • buildAliasChecks
  • hoistInvariantLoads
  • canonicalizeDynamicBasePtrs
  • verifyInvariantLoads