[WIP] Visitor for AccessedStorage use-def chain walk

[jckarter]

Factor a visitor out of findAccessedStorage to let other clients of the logic have more control over what's done with the information. Use the visitor in the isWrittenTo analysis to extract the exact begin_borrow that an access went through in order to do the linear lifetime analysis on let loads.

[jckarter]

@swift-ci Please benchmark

[jckarter]

@swift-ci Please test

[gottesmm]

Two quick comments. I want to go through this with a finer tooth comb again.

[gottesmm]

Why not use CRTP and make visitAccessUseDefChain a static method?

[gottesmm]

Does this expand the functionality of the pass in some way? If so, can you do that in a different PR? My hope is that this one can be just a simple refactoring so I can eye-ball it.

[jckarter]

The refactoring part is in 918282e6a80619e072be6b1d8f186f7ab38926e3, and the SemanticARCOpts bit in the next commit, if you want to review them separately.

[gottesmm]

Ah ok. I didn't notice it was in a separate commit.

[atrick]

It might be better to split AccessedStorage into an AccessedStorageKind interface with the uint_8 enum and all the kind-related queries.

[atrick]

I think this needs to be a template parameter. findAccessedStorage[NonNested] is the critical path for several expensive passes. Every time I debugged a compile time issue with exclusivity enabled, all the time was spent in getAccessedStorageFromAddress. I wouldn't don't mind keeping it in the .cpp and explicitly specializing for the few visitors, but that could mean including SILOptimizer headers from within SIL.

The template could be instantiated once for getAccessedStorageFromAddress and a second time for a virtual visitor, but maybe it should just be fast everywhere.

[jckarter]

Maybe we could make the implementation of visitAccessUseDefChain always_inline within MemAccessUtils.cpp, so that the visitor can be devirtualized.

[atrick]

You don't need the worklist in the outer function anymore. If you move the class into an anonymous namespace, then it would still be possible to read the outer function.

[jckarter]

Oops, I forgot to delete the now-unused vars from the outer scope. What do you mean by " it would still be possible to read the outer function". At top level, the class would still need to declare its own member variables for the state, unless I'm missing something.

[atrick]

This is just my personal bias toward function scopes that fit on screen. I can read this:

namespace {
class Utility {
  State state;
  visitStuff1() {}
  visitStuff2() {}
};
} // namespace

void myAPI() {
  // has an implementation that fits on a screen
  return Utility().doIt()
}

I can't read this:

void myAPI() {
  class {
   // blah blah for more than a screen
  }
  // who does this code belong too a method of the above class or some outer function?
  return stuff
}

[jckarter]

Ah, I thought you mean "read" in a language semantics sense. OTOH, scoping the nested class helps indicate that there are no uses of the class outside of the one function, and there's only one line of code outside of the class in this case.

[swift-ci]

Performance: -O

Code size: -O

Performance: -Osize

Improvement	OLD	NEW	DELTA	RATIO
PrefixCountableRangeLazy	17	15	-11.8%	1.13x (?)

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

[jckarter]

It might be better to split AccessedStorage into an AccessedStorageKind interface with the uint_8 enum and all the kind-related queries.

@atrick Could you clarify what you mean?

[atrick]

When I wrote the comment about AccessedStorageKind, you still had a visitIdentified. Which might actually be what we want most of the time. It's nice to be able to write a small covered switch rather than override a bunch of visitors--and only override a visitor when you need to specially handle borrows for example. Here's a quick strawman, not sure it makes sense without trying to implement it:

class AccessedStorage {
  enum Kind { ...}
  static classify(SILValue)
  Kind kind
  SILValue base
  bool isLocal()
  isUniquelyIdentified()
//etc.
}

class AccessedStorageKey {
  AccessedStorage::Kind kind
  int64 opaqueBits
  union { SILValue, Global }
  getArgument()
  getObject()
  bool hasIdenticalBase()
  ...
}

AccessedStorage findAccessedStorage<Visitor>(SILValue) {...}

AccessedStorageKey findAccessedStorageKey(SILValue V) {
  return AccessedStorageKey(findAccessedStorage<RecurseThroughPhisAndStrongAssertions>(V))
}

[jckarter]

Hm. I split out the visitor methods because IMO it helped make clear the almost 1:1 type relationship between access kinds and the instruction types they correspond to—that was lost on me when I was first looking at this code—and it cleaned up a bunch of casts that were necessary in the SemanticARCOpts code before I broke out the methods. If we're going to CRTP this, I could make it so that the individual access methods all have default implementations that call a common visitIdentified method, which I think would get the result you want—in the common case, you only need to implement one method, and you only need to implement the more specific methods you actually need.

[jckarter]

@swift-ci Please test

[jckarter]

@atrick @gottesmm OK, I CRTP-ized the visitor, and routed the methods through a visitBase common method by default.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 127198b81132f3045fb859ab15d889a2114a8e3d

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 127198b81132f3045fb859ab15d889a2114a8e3d

[atrick]

@jckarter this looks really nice! Yes, visitors should have typed cases--that's the only time I like the visitor pattern. The user can just construct an AccessedStorage from the identified base to get all the benefits I was looking for. I was trying to micro-optimize by classifying AccessedStorage first but this is cleaner and probably just as fast.