[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)
----------------
nikic wrote:
> barannikov88 wrote:
> > 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 ...
> The variant on `Value` would accept a `Function *`, which would allow handling constants as well (for them, the insertion point is the same as for arguments: the start of the function).
> 
> I don't think this has any particular relation to basic blocks, so adding to BasicBlock or BasicBlockUtils does not seem appropriate to me.
Well, polluting Value with an interface which is used in exactly four places it not a solution either. That's why I'm suggesting to make this a standalone function, not a member of any particular class. Where to put it is a separate question, but since it is used in transformation passes it may make sense to put it somewhere under include/llvm/Transforms/.
Please consider my comments as a suggestion only.



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

https://reviews.llvm.org/D129660