Swift Optimizer: add def-use/use-def walker utilities

[Angelogeb]

This PR adds def-use/use-def traversals in WalkUtils.swift and updates EscapeInfo and related passes to use those.

WalkUtils adds a set of protocols (ValueDefUseWalker, AddressDefUseWalker, ValueUseDefWalker, AddressUseDefWalker) that perform def-use and use-def traversals guided by a projection path.
An implementor of these protocols can collect uses/definitions and track whether
certain instructions have been crossed during the traversal.

Additionally EscapeInfo and related passes are updated to use the traversal utilities.

[eeckstein]

@swift-ci test

[eeckstein]

@swift-ci benchmark

[eeckstein]

nice!
lgtm

[Angelogeb]

@swift-ci test

[Angelogeb]

@swift-ci test

[atrick]

"Base" value

We have a specific definition of a base address in SIL. Is there any need to overload the term to mean the initial value for a traversal? Can we just call it an "initial" value?

leafUse

  mutating func leafOrUnmatched(value: Operand, path: Path, state: State) -> Bool {
    if path.isEmpty { return leafUse(value: value, path: path, state: state) }
    else { return unmatchedPath(value: value, path: path, state: state) }
  }

I don't follow this logic. unmatchedPath is simultaneously being used to mean different things. We have to decide whether it means:

(1) Disjoint paths

The unmatchedPath API should only be called after proving that the paths are disjoint. i.e. the unmatchedPath values are disjoint from the specified path.

(2) Incomplete path

Here "unmatchedPath" simply means "not fully matched". So it is indistinguishable from any call to leafUse with a non-empty path. In this case leafUse should only be called when the path is empty. It should not take a path argument at all.

index_addr

Why is index_addr treated as a cast? Until SmallProjectionPath supports indices, it needs to be a "leafOrUnmatched" case. Just like tail_addr.

switch_enum

It doesn't look like you handle the switch_enum case yet.

Bool return

When an API performs an action and returns a boolean. The return value should indicate whether the action was succesfull. That way code using the API can be read logically:

if doSomething() {
  // assume it was done and read the results
}

if !doSomething() {
  // bail-out here
  return false
}

(Some parsing APIs reverse this polarity, which is an abhorrent carry-over from C integer error codes. I can't imagine we would carry that practice over to Swift API. Do you know of any Swift APIs where 'false' indicates that the action succeeded?)

At any rate, SIL transformations all follow this convention for bailing out.

We also have many visitor patterns in the SIL code base. Here, a false return value always short-circuits graph traversal. So we have:

if visit(node) {
  // continue graph traversal
}

if !visit(node) {
  return false // bail-out of graph traversal.
}

Some visitor clients may combine the "unrecognized graph" condition with a "failed check" condition to avoid maintaining extra state. This is fine as long as false still corresponds to the condition that short-circuits graph traversal at an "unrecognized graph".

What is important here is that the sense of the returned boolean cannot change based on the how the client chooses to interpret the result of the traversal.

So if the walker is used underneath an API that checks for a negative condition, it would logically invert the result:

func isEscaping() {
  if !walkDown() {
    // if the walk did not complete, then the value conservatively escapes.
    return true
  }

Flipping the meaning of a boolean to suit whatever was "convenient" to the API implementer is actually an extraordinarily common source of bugs.

reflexivity

This is a silly thing to point out. But would it makes sense to define AddressDefUseWalker.walkDownDefault and
AddressUseDefWalker.walkUpDefault inside extensions back-to-back since they need to mirror each other?

[eeckstein]

Thanks, @atrick for reviewing. Here's my feedback on your comments.

[eeckstein]

Why is index_addr treated as a cast? Until SmallProjectionPath supports indices, it needs to be a "leafOrUnmatched" case. Just like tail_addr.

Well, conceptually SmallProjectionPath already "handles" index_addr. It just unconditionally matches all indices.
tail_addr is a leaf node because it is a transition from value to address, while index_addr is completely within an address chain.

But it's probably a good idea to add a comment here to describe this and add a TODO to better support index_addr in SmallProjectionPath.

[eeckstein]

It doesn't look like you handle the switch_enum case yet.

That's a good point. We could move the handling of switch_enum into the walkers. And of course also in the down-walk.

[eeckstein]

The return value should indicate whether the action was succesfull.

I agree.

It would be even better to return an enum instead of a Bool, e.g.

enum WalkResult {
  case continueWalk
  case abortWalk
}

Returning a Bool is fine for functions which imply the meaning by it's name, e.g. isEscaping.
But for functions like walkDown, it's better to return an enum, which makes the meaning of the returned value immediately clear.

[eeckstein]

@swift-ci test