[PATCH] D88872: [Coroutines] Refactor/Rewrite Spill and Alloca processing

[John McCall via Phabricator via llvm-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

This is a nice cleanup, thank you.  Just minor comments.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:721
+    // PromiseAlloca field needs to be explicitly added here because it's
+    // not in FrameData.Allocas.
+    if (PromiseAlloca)
----------------
Well, it needs to be explicitly added here because it's a header field which has to have a fixed offset based on its alignment, which is *why* it's not in `FrameData.Allocas`.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:752
   case coro::ABI::Switch:
+    // In the switch ABI, remember the field indices for the switch-index field.
     Shape.SwitchLowering.IndexField =
----------------
It's just "index" now.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1957
+      else
+        ShouldLiveOnFrame = Checker.isDefinitionAcrossSuspend(I, U);
+
----------------
I see that this seems to be the existing algorithm, and I know this is intended to be a refactoring patch rather than a functional one, so I'll let this go without comment beyond expressing my eagerness to see a follow-up. :)


================
Comment at: llvm/lib/Transforms/Coroutines/CoroInternal.h:125
     unsigned IndexField;
-    unsigned PromiseField;
     bool HasFinalSuspend;
----------------
Yeah, I think this is never used directly — clients expect to find it by offset, and within the function it's of course just referenced as a value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88872