[PATCH] D73832: Ignore/Drop droppable uses for code-sinking in InstCombine

[Tyker via Phabricator via llvm-commits]

Tyker marked an inline comment as done.
Tyker added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:263
+      Assume.bundle_op_infos(), [&Assume](const CallBase::BundleOpInfo &BOI) {
+        return BOI.Begin == BOI.End ||
+               getValueFromBundleOpInfo(Assume, BOI, BOIE_WasOn) != nullptr;
----------------
jdoerfert wrote:
> Tyker wrote:
> > jdoerfert wrote:
> > > We do we disallow empty BundleOpInfo here?
> > i am not sure i understood the question.
> > 
> > we will return true for empty bundles.
> > 
> > this is a simple utility to check if the operand bundle of an llvm.assume still holds any information. it is used in a few places because instcombine was removing instructions like `call void @llvm.assume(i1 true) ["nonnull"(%p)]` because the argument to the call is known to be true so the assume was considered useless which is not true anymore.
> > 
> > 
> Oh, empty bundles like `call void @llvm.assume(i1 true) ["cold"()]` ?
> Maybe we should put a comment here because I will forget again.
> 
> Nit: no `!= nullptr`
`call void @llvm.assume(i1 true) ["cold"()]` is not considered empty.

this function returns true for truly empty/absent bundles like `call void @llvm.assume(i1 true)`.
or for bundles for which there was `WasOn` arguments but they have all been dropped. for example if we had `call void @llvm.assume(i1 true) ["nonnull"(%p)]`  and the use of %p get gets dropped. all the only information the assume contains is that a value used to be nonnull, which is basically useless. so the function returns true for those bundles as well.

the general rule is this function returns true if the operand bundle of the assume doesn't hold any usable information. so if this function returns true and the argument of the assume is always true the whole assume should be removed.

i realize that this function probably deserve a more detailed comment.


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

https://reviews.llvm.org/D73832