[PATCH] D94137: [coroutine] update promise object's final layout index

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:750
       FrameData.setFieldIndex(
           PromiseAlloca, B.addFieldForAlloca(PromiseAlloca, /*header*/ true));
 
----------------
lxfind wrote:
> ychen wrote:
> > lxfind wrote:
> > > PromiseAlloca is added here as a header field, which means it should have a fixed index?
> > > Could it be there are bugs in performOptimizedStructLayout that moved it around somehow?
> > I'm inclined to say it should be fixed offset which is not changed after `performOptimizedStructLayout`.
> > 
> > 1) The use of `IsHeader` in addFieldForAllocas suggests that.
> > 2) The comment below in OptimizedStructLayout.h
> > 
> > ```
> > /// - Fields may be assigned a fixed offset in the layout.  If there are
> > ///   gaps among the fixed-offset fields, the algorithm may attempt
> > ///   to allocate flexible-offset fields into those gaps.  If that's
> > ///   undesirable, the caller should "block out" those gaps by e.g.
> > ///   just creating a single fixed-offset field that represents the
> > ///   entire "header".
> > ```
> > 
> You are right.
> In that case, I wonder if it is still necessary to add PromiseAlloca as a header field?
Yeah, that's an option too.  TBH, I'm not quite sure about the importance or implication of PromiseAlloca using flexible offset. Or maybe it would make `llvm.coro.promise` implementation hard or impossible?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94137