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

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 13 12:06:01 PDT 2022


nikic 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:
> 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.


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

https://reviews.llvm.org/D129660



More information about the llvm-commits mailing list