[move-function] Implement move checking for address only lets

[gottesmm]

This new pass works as follows:

1. Mandatory Inlining converts builtin "move" to a mark_unresolved_move_addr when inlining the function "_move" (the only place said builtin is invoked). mark_unresolved_move_addr is semantically a copy_addr [init] so we have valid IR.

2. This is then run through a special checker that
   either proves that the mark_unresolved_move_addr can actually be a move in which
   case it converts it to copy_addr [take][init] or if it can not be a move, emit
   an error and convert the instruction to a copy_addr [init]. After this is done
   for all instructions, we loop back through again and emit an error on any
   mark_unresolved_move_addr that were not processed earlier allowing for us to
   know that we have completeness.

NOTE: The move kills checker for addresses runs after Mandatory
Inlining, but before predictable memory opts and any other memory optimizations on purpose.

[gottesmm]

@swift-ci test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 5d6c72a258e313c6cde7cf340c925e50a90530f5

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 5d6c72a258e313c6cde7cf340c925e50a90530f5

[gottesmm]

@swift-ci test

[gottesmm]

@swift-ci test

[atrick]

This seems like the best place to put a review of this pass even though it's based on the latest iteration of the code.

Notes on the design description

The generally looks like a great approach from the high level description. It's should be as similar to the value-based algorithm as possible.

The same diagnostic algorithm should be used for moveonly variables and copyable variables that are explicitly moved. The name indicates otherwise. I expected simply a MoveValueCheck.cpp and MoveAddressCheck.cpp.

As discussed offline, the complete form of this diagnostic is equivalent to DefiniteInitialization. DI can insert mark_initialized markers to help you, but that's a pretty big FIXME to call out.

Please check that the design handles all the variations on indirect call arguments. I'm a little concerned that SILGen might emit temporary alloc_stack copies for some things.

semantically this instruction is actually a copy_addr [init]

Why is mark_unresolved_move_addr equivalent to copy_addr [init]? What does the [init] have to do with the move? Do you mean copy_addr [take]?

Then it partitions those uses into three sets:

It all be simpler to understand if you partition uses into two sets: live uses vs. destroys. Then say that some subset of those live uses are "moves".

NOTE: We do not need to check for
moves here since any move in our block would have either resulted in the
destroy_addr being eliminated earlier by the single block form of the
diagnostic and us exiting early or us emitting an error diagnostic and
exiting early.

This is hard to parse. Isn't it just this?

We don't check for moves because a move followed by a destroy
without an intervening init is invalid SIL, caught be the memory
verifier.

NOTE: The reason why we do not track init information on a per block is that
in our dataflow, we are treating inits as normal uses and if we see an init
without seeing a destroy_addr, we will error on the use and bail. Once we
decide to support vars, this will change.

I don't understand what "see an init without seeing a destroy_addr" means. But It seems unfortunate to have an algorithm for addressable variables that doesn't support reinitialization.

NOTE: The reason why we do not track all consuming operations

For moveonly variables, consuming operations will need to be handled just like moves. I'm not sure why the algorithm isn't just designed that way from the start. This whole discussion and design is centered on the mark_unresolved_move_addr opcode. The only thing that's special about that instruction is that it should be treated like a consume even though it's OperandOwnership isn't consuming (yet). I don't think the design should be centered around that operation.

If b is a block associated with one of our converted
mark_unresolved_move_addr, continue.

When you say "continue" here I think you mean "stop".

Notes on the implementation

There are several different data flow algorithms and dozens of data structures, without clear boundaries between them. Each aspect of the pass should be encapsulated into its own internal state distinct from its result that feeds into other aspects:

• Gather function uses
• Upward liveness data flow
• Downward exposed consume data flow
• Closure use summary (based on those 2 data flows)
• Scanning for local use-after-move
• Destroy placement data flow
• Diagnostic handling

For each data flow, clearly separate:

• Seeding data flow
• Basic block summary (based on local scan)
• Global data flow propagation
• Data flow results

Most of the large functions in this pass are trying to do multiple unrelated things at once and have access to all different pieces of state. These are a ton of data structures across this code. They should all be encapsulated into objects that are each the output of one stage and the input of following stages.

struct GatherClosureUseVisitor : public AccessUseVisitor {

This is easy to understand. But UseState has a large number of data structures that are directly exposed to the entire pass.

Please clarify somehow that UseState is the output of GatherClosureUseVisitor. The only modification to that output is that uses are "cleared" later as they are processed. The UseState and GatherClosureUseVisitor are currently separated by 500 lines of code.

  SmallVector<MarkUnresolvedMoveAddrInst *, 1> markMoves;
  SmallPtrSet<SILInstruction *, 1> seenMarkMoves;
  SmallSetVector<SILInstruction *, 2> inits;
  SmallSetVector<SILInstruction *, 4> livenessUses;
  SmallBlotSetVector<DestroyAddrInst *, 4> destroys;
  llvm::SmallDenseMap<SILInstruction *, unsigned, 4> destroyToIndexMap;
  SmallBlotSetVector<SILInstruction *, 4> reinits;
  llvm::SmallDenseMap<SILInstruction *, unsigned, 4> reinitToIndexMap;
  llvm::SmallMapVector<Operand *, ClosureOperandState, 1> closureUses;
  llvm::SmallDenseMap<Operand *, unsigned, 1> closureOperandToIndexMap;

Should look like

  // definition of this...
  SmallVector<MarkUnresolvedMoveAddrInst *, 1> markMoves;

  // definition of this...
  SmallPtrSet<SILInstruction *, 1> seenMarkMoves;

  // definition of this...
  SmallSetVector<SILInstruction *, 2> inits;

  // definition of this...
  SmallSetVector<SILInstruction *, 4> livenessUses;

  // definition of this...
  SmallBlotSetVector<DestroyAddrInst *, 4> destroys;
  llvm::SmallDenseMap<SILInstruction *, unsigned, 4> destroyToIndexMap;

  // definition of this...
  SmallBlotSetVector<SILInstruction *, 4> reinits;
  llvm::SmallDenseMap<SILInstruction *, unsigned, 4> reinitToIndexMap;

  // definition of this...
  llvm::SmallMapVector<Operand *, ClosureOperandState, 1> closureUses;
  llvm::SmallDenseMap<Operand *, unsigned, 1> closureOperandToIndexMap;

Why do you have both SmallBlotSetVector/SmallMapVector and DenseMap for the same thing?

You can write nullptr into a vector instead of using Blot/MapVector. The null-ness can be hidden behind the API.

I don't know why you need bitsets. But, if you do map instructions to integers, why aren't you just using a single instruction map, a single vector for all uses, and 6 bitsets to represent the sets? Instead of 9 vectors, 9 hashmaps, and dynamically populated bitsets?

None of these data structure details should be exposed. You can add an API inside UseState like:

clearDestroy(Destroy *)

Bitset &getDestroyBits()

static DownwardScanResult
downwardScanForMoveOut(MarkUnresolvedMoveAddrInst *mvi, UseState &useState,
SILInstruction **foundInst, Operand **foundOperand,

APIs like this are hard to make sense of. An analysis function takes some input and produces a result. DownwardScanResult, foundInst, foundOperand, and foundClosureInsts can all be encapsulated in DownwardScanResult.

ClosureArgDataflowState

I could not figure out how the closure analysis fits with the rest of the pass. It seems like exactly the same analysis should run on the main function and on the closure, then two separate pieces of information need to be summarized for the closure:

(1) upward liveness

(2) downward exposed consumes

downwardScanForMoveOut()...
auto iter = useState.reinitToIndexMap.find(&next);
if (iter != useState.reinitToIndexMap.end()) {

Code like this makes the function hard to read. This function doesn't care how the data structure is implemented. It should just ask useState.isReinit(inst).

DataflowState::init() and upwardScanForXXX()...

It might be simpler and more efficient to have a single BasicBlockSet for all blocks with uses (from GatherUses). And have a single upwardScan function that runs for each block initializes the block's data flow summary.

performSingleBasicBlockAnalysis(...

performSingleBasicBlockAnalysis conceptually has nothing to do with global data flow propagation. DataflowState should not be passed to it.

dataflowState.markMovesThatPropagateDownwards.emplace_back(mvi);
// Now scan up to see if mvi is also a use to seed the dataflow. This could
// happen if we have an earlier move.
if (upwardScanForUseOut(mvi, dataflowState.useState)) {
LLVM_DEBUG(llvm::dbgs() << "MVI projects a use up");
dataflowState.useBlocks[mvi->getParent()] = mvi;
}

This upward scan here seems unrelated to the purpose of performSingleBasicBlockAnalysis. All the per-block data flow initialization should be encapsulated in a single place.

Generally, try to avoid conflating the move-as-use vs. the move-as-lifetime-end. Just handle move-as-use as part of the regular use set.

{
auto diag =
diag::sil_movekillscopyablevalue_value_consumed_more_than_once;
StringRef name = getDebugVarName(address);
diagnose(astCtx, getSourceLocFromValue(address), diag, name);
}

Can you centralize all the diagnostics in a single object? Then, instead of passing in address, just pass in the diagnostic handler? The interface can be:

/// True if \p move is exposed at the end of its block without first being destroyed.
bool checkUseAfterMoveInBlock(SILInstruction *move, ErrorHandler &handler) {...}

In DataflowState::process, I can't figure out why bitsets are suddently used here. What does it means to "pair" an instruction, and why we would care about "pairing".

bool DataflowState::cleanupAllDestroyAddr(...

I don't understand why cleanupAllDestroyAddr is a member of DataflowState. It takes 8 arguments. Most of them data structures defined locally then passed around. These should all be encapsulated in a separate data flow algorithm for destroy placement.

It would be really great if each data flow was a separate class that only contained data flow state and data flow logic and produced a data flow result. Nothing else whatsoever. Otherwise I think it's practically impossible to relate the code to the file-level comment that explains the design.