[all-commits] [llvm/llvm-project] 667dfe: [Coroutines] Refactor/Rewrite Spill and Alloca pro...

[Xun Li via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 667dfe39caa0023b1d00b3e126c7df57702aaf14
      https://github.com/llvm/llvm-project/commit/667dfe39caa0023b1d00b3e126c7df57702aaf14
  Author: Xun Li <xun at fb.com>
  Date:   2020-10-10 (Sat, 10 Oct 2020)

  Changed paths:
    M llvm/lib/Transforms/Coroutines/CoroFrame.cpp
    M llvm/lib/Transforms/Coroutines/CoroInternal.h
    M llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
    M llvm/test/Transforms/Coroutines/coro-debug.ll
    M llvm/test/Transforms/Coroutines/coro-frame-arrayalloca.ll
    M llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-01.ll
    M llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-02.ll
    M llvm/test/Transforms/Coroutines/coro-retcon-frame.ll
    M llvm/test/Transforms/Coroutines/coro-retcon-once-value2.ll
    M llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-01.ll
    M llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-03.ll
    M llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-04.ll

  Log Message:
  -----------
  [Coroutines] Refactor/Rewrite Spill and Alloca processing

This patch is a refactoring of how we process spills and allocas during CoroSplit.
In the previous implementation, everything that needs to go to the heap is put into Spills, including all the values defined by allocas.
And the way to identify a Spill, is to check whether there exists a use-def relationship that crosses suspension points.

This approach is fundamentally confusing, and unfortunately, incorrect.
First of all, allocas are always process differently than spills, hence it's quite confusing to put them together. It's a much cleaner to separate them and process them separately.
Doing so simplify lots of code and makes the logic more clear and easier to reason about.

Secondly, use-def relationship is insufficient to decide whether a value defined by AllocaInst needs to go to the heap.
There are many cases where a value defined by AllocaInst can implicitly be used across suspension points without a direct use-def relationship.
For example, you can store the address of an alloca into the heap, and load that address after suspension. Or you can escape the address into an object through a function call.
Or you can have a PHINode that takes two allocas, and this PHINode is used across suspension point (when this happens, the existing implementation will spill the PHINode, a.k.a a stack adddress to the heap!).
All these issues suggest that we need to separate spill and alloca in order to properly implement this.
This patch does not yet fix these bugs, however it sets up the code in a better shape so that we can start fixing them in the next patch.

The core idea of this patch is to add a new struct called FrameDataInfo, which contains all Spills, all Allocas, and a map from each definition to its layout index in the frame (FieldIndexMap).
Spills and Allocas are identified, stored and processed independently. When they are initially added to the frame, we record their field index through FieldIndexMap. When the frame layout is finalized, we update each index into their final layout index.

In doing so, I also cleaned up a few things and also discovered a few other bugs.

Cleanups:
1. Found out that PromiseFieldId is not used, delete it.
2. Previously, SpillInfo is a vector, which is strange because every def can have multiple users. This patch cleans it up by turning it into a map from def to users.
3. Previously, a frame Field struct contains a list of Spills that field corresponds to. This isn't necessary since we only need the layout index for each given definition. This patch removes that list. Instead, we connect each field and definition using the FieldIndexMap.
4. All the loops that process Spills are simplified now because we use a map instead of a vector.

Bugs:
It seems that we are only keeping llvm.dbg.declare intrinsics in the .resume part of the function. The ramp function will no longer has it. This means we are dropping some debug information in the ramp function.

The next step is to start fixing the bugs where the implementation fails to identify some allocas that should live on the frame.

Differential Revision: https://reviews.llvm.org/D88872