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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2640
+    else if (isa<Argument>(Storage))
+      InsertPt = &*F->getEntryBlock().begin();
+    if (InsertPt)
----------------
barannikov88 wrote:
> nikic wrote:
> > I'm wondering if it makes sense to move this API to `Value` instead, to handle this case as well. It's needed here and in the InstCombine freeze transform.
> There are other Values apart from Argument and Instruction for which the term "point [within a basic block] after the value is defined" does not make sense (e.g. Constant), . It of course could be asserted, but still, Value seems too low-level and too fundamental class for this kind of interface.
> I'm not even sure the Instruction is a good place for this method either. The method always returns a location within some basic block, so putting this method into a BasicBlock might seem like a good idea. However, the method should be made static, which would just overload the class with rarely used method with no real benefit.
> Considering all of this, I would suggest to turn this method into a standalone utility function with a template argument which is statically asserted to be an Instruction or an Argument.
> llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h might be a good place.
> 
> Excuse my English
or llvm/include/llvm/Transforms/Utils/Local.h or ...


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

https://reviews.llvm.org/D129660