[PATCH] D87596: [Coroutines] Reuse storage for local variables with non-overlapping lifetimes

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:484
+  ///
+  /// Side Effects: Because We use `DenseMap` to record the allocas, the order
+  /// of allocas in the frame may be different with the order in the source
----------------
lxfind wrote:
> ChuanqiXu wrote:
> > lxfind wrote:
> > > ChuanqiXu wrote:
> > > > lxfind wrote:
> > > > > What is this referring to? Allocas in this function seems to be `SmallVector` type.
> > > > We add the field for allocas in `addFieldForAllocas`, where the allocas are in `DenseMap`. I think it is more clear to move the comment to `addFieldForAllocas` function.
> > > I see. In that case, my understanding is that the order of allocas in the frame may be different with the order in the source code not because we use DenseMap but because we sort using the size.
> > Yes. In my experiments, the mismatch order happens after I sort the allocas. I think DenseMap may also be responsible to this mismatch in some conditions. Because we add the field for allocas by such way:
> > ```
> > for (Set in NonOverlappedSet)
> >     add field for first alloca in the Set
> > ```
> > Because `NonOverlappedSet` is a `DenseMap`, the order we add field is not guaranteed. I think both the sorts and the `DenseMap` are responsible for the mismatch. I would note this in the comment later.
> hmm good point. In that case we have a real problem here. We would want the compilation result to be deterministic, which means we need a way to guarantee the order when iterating the map.
> Why is `NonOverlapedAllocas` a `DenseMap` though? Looking at the way you are adding elements to it, seems like it can very well just be a `SmallVector`.
The compilation result is deterministic although we can't get the result  by just a watch. I think it is a good idea to let `NonOverlappedSet` be a `SmallVector`. However, as we would sort the allocas by size, the order in the frame would still mismatch the order in the source. I think the order of allocas in the frame may not matter a lot.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87596/new/

https://reviews.llvm.org/D87596