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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/IR/AsmWriter.cpp:2557
 
+    /// Bundles containing dropped values should be ignored.
+    if (any_of(BU.Inputs, [](const Use &U) { return !U.get(); }))
----------------
Tyker wrote:
> Tyker wrote:
> > lebedev.ri wrote:
> > > Would it be better to ensure that we can't end up in such
> > > seemingly-degenerate state in the first place,
> > > by cleaning up bundles?
> > i agree than not having that state would be better but i don't think it is possible without rebuilding instruction after we dropped uses in them.
> > 
> > an other solution could be to mark dropped uses with an undef value instead of a nullptr.
> i tried to find a better solution to represent "dropped" values. but not solution seems to be good.
>  - nullptr is problematic because many place assume that operands are never null.
>  - rebuilding instructions seems like too expensive for what we are trying to do.
> 
> maybe adding a new kind of value to represent it, that is ignored everywhere. but changing the IR for this seems like overkill.
> 
> any thought ?
I like the undef solution if we clarify in the LangRef that an assume on `undef` is ignored.

If we could change the bundle tag we could set it to "ignore", and then pass `undef`.


================
Comment at: llvm/test/Transforms/InstCombine/assume.ll:274
 ; CHECK:       taken:
-; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i32* [[LOAD]], null
 ; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP]])
----------------
Tyker wrote:
> jdoerfert wrote:
> > Why don't we sink this one anymore?
> in this case there is a single use which is droppable (in an llvm.assume). so hasOneUse return true, but getSingleUndroppableUse returns null. so the old passe will sink the instruction whereas the new won't.
> 
> this doesn't matter however because this instruction is only used in an @llvm.assume so it is already dead and the backend will it cleanup.
I see.


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

https://reviews.llvm.org/D73832