[PATCH] D129660: [IR] Add Instruction::getAfterDefInsertionPoint()

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.
Herald added a subscriber: ChuanqiXu.

I would strongly prefer to see this split into a NFC change (even if that means preserving a bug), and functional changes to fix each bug in turn.

In fact, LGTM if you want to land the API with a non-functional change user.  But only in that case.

I sort of wonder if we could handle the Argument case by having this be a utility on Function, but we can explore that in a future revision if that patterns turns out to be common.



================
Comment at: llvm/include/llvm/IR/Instruction.h:157
+  /// e.g. due to a catchswitch terminator.
+  Instruction *getAfterDefInsertionPoint();
+
----------------
Can you change this to getInsertionPointAfterDef?  That reads more clearly to me.  


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2641
+      InsertPt = &*F->getEntryBlock().begin();
+    if (InsertPt)
+      DVI->moveBefore(InsertPt);
----------------
I think you should assert InsertPt here.  Otherwise, we have to figure out what to do with DVI in the catch switch case which the existing code didn't handle.  


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

https://reviews.llvm.org/D129660