[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/Passes/PassBuilder.cpp:1710-1715
     int OptLevel = StringSwitch<int>(ParamName)
                        .Case("O0", 0)
                        .Case("O1", 1)
                        .Case("O2", 2)
                        .Case("O3", 3)
                        .Default(-1);
----------------
lxfind wrote:
> ChuanqiXu wrote:
> > lxfind wrote:
> > > I think you can use the same code like this, which appears to be simpler.
> > I am wondering how should I handle unknown ParamName in this style. Should I just skip it? Or should I return an error? which implies that we should add a new static member to `PassBuilder::OptimizationLevel` as `PassBuilder::OptimizationLevel::Unknown`.
> To be honest I don't think coroutine requires a fine-grained optimization level control from 0-3. So it would be sufficient if we just use a bool to indicate whether it will be optimized. O1,O2,O3 will set it to true, otherwise it can stay false.
> Alternatively, if you prefer to use `OptimizationLevel`, I think it's fine to set it to O0 if it's unknown level.
> 
> However, at a high level, I am not sure if it's worth all the trouble adding all those changes to PassBuilder just to be able to specify the syntax `coro-split<O2>` to `opt` tool. Since you already added a separate flag to control this, which is sufficient to test `opt`, as long as the optimization is gated when it's invoked through Clang optimization pipeline, I would say it's good enough.
I think your suggestion is right. The coroutine split pass don't need a fine-grained optimization control mechanism now.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:360
     uint64_t Offset;
-    Spill *ForSpill;
+    SmallVector<Spill *, 8> ForSpill;
     Type *Ty;
----------------
lxfind wrote:
> Use `ForSpillType`?
thanks, done


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:389
   /// instruction.
   FieldId addFieldForAlloca(AllocaInst *AI, Spill *ForSpill = nullptr,
                             bool IsHeader = false) {
----------------
lxfind wrote:
> I think we can just define one version of `addFieldForAlloca` with `ForSpillType ForSpills = {}`
done


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:445
   /// Add a field to this structure.
   FieldId addField(Type *Ty, MaybeAlign FieldAlignment,
                    Spill *ForSpill = nullptr,
----------------
lxfind wrote:
> Do we need 2 versions of `addField`? Would it work if we set `ForSpillType ForSpill = {}` as the parameter?
done


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

https://reviews.llvm.org/D87596