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

[Xun Li via Phabricator via llvm-commits]

lxfind 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);
----------------
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.


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


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


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


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

https://reviews.llvm.org/D87596