[llvm-dev] InstCombine doesn't delete instructions with token

[Alexandre Isoard via llvm-dev]

Seems you are right, looks like:

@llvm.call.preallocated.arg(token, i32)

requires its token to be from a

@llvm.call.preallocated.setup(i32)

Those are the only token related check in the verifier that are not EHPad.


On Wed, Jun 17, 2020 at 1:07 PM Eli Friedman <efriedma at quicinc.com> wrote:

> Huh, I really thought undef tokens weren’t legal; I’ve never seen one.
>
>
>
> I’m still concerned about verifier checks for specific intrinsics; in some
> cases, we expect that the operand is some specific instruction/intrinsic,
> not none/undef.
>
>
>
> -Eli
>
>
>
> *From:* Alexandre Isoard <alexandre.isoard at gmail.com>
> *Sent:* Wednesday, June 17, 2020 1:01 PM
> *To:* Eli Friedman <efriedma at quicinc.com>
> *Cc:* David Majnemer <david.majnemer at gmail.com>; llvm-dev at lists.llvm.org
> *Subject:* [EXT] Re: [llvm-dev] InstCombine doesn't delete instructions
> with token
>
>
>
> I did not observe any assertion. In addition, the documentation (
> https://llvm.org/docs/LangRef.html#undefined-values) says:
>
>
>
>  The string ‘undef’ can be used anywhere a constant is expected, and
> indicates that the user of the value may receive an unspecified
> bit-pattern. Undefined values may be of any type (other than ‘label’ or ‘
> void’) and be used anywhere a constant is permitted.
>
>
>
> Either way, using a 'none' token instead is fine.
>
>
>
> For an example: https://godbolt.org/z/MowxS_
>
>
>
> Where the following input:
>
>
>
> define void @foo() #0 {
>
> entry:
>
>     unreachable
>
>
>
> exit:
>
>     %tok = call token @llvm.bar()
>
>     call void @llvm.foo(token %tok)
>
>     call void @llvm.foo(token none)
>
>     call void @llvm.foo(token undef)
>
>     ret void
>
> }
>
>
>
> attributes #0 = { norecurse nounwind readnone }
>
>
>
> Will produce after instcombine:
>
>
>
> ; Function Attrs: norecurse nounwind readnone
>
> define void @foo() #0 {
>
> entry:
>
>   unreachable
>
>
>
> exit:                                             ; No predecessors!
>
>   %tok = call token @llvm.bar()
>
>   ret void
>
> }
>
>
>
> attributes #0 = { norecurse nounwind readnone }
>
>
>
> Where the remaining call is a wasted instruction.
>
>
>
> On Wed, Jun 17, 2020 at 12:28 PM Eli Friedman <efriedma at quicinc.com>
> wrote:
>
> There’s no such thing as an “undef” token; you’ll get an assertion if you
> try to create one.  There is “none”, but the verifier will fail in some
> cases if it sees a “none” token.
>
>
>
> In terms of actually erasing instructions, it’s specifically EHPad we
> can’t erase: unwind edges are required to have an EHPad instruction, so
> erasing them requires modifying the block’s predecessors.  We don’t need to
> keep an intrinsic call with no uses that produces a token result.
>
>
>
> -Eli
>
>
>
> *From:* Alexandre Isoard <alexandre.isoard at gmail.com>
> *Sent:* Tuesday, June 16, 2020 10:25 PM
> *To:* Eli Friedman <efriedma at quicinc.com>
> *Cc:* David Majnemer <david.majnemer at gmail.com>; llvm-dev at lists.llvm.org
> *Subject:* [EXT] Re: [llvm-dev] InstCombine doesn't delete instructions
> with token
>
>
>
> Yes, it's still respected in this case, as the only instructions that will
> be deleted have been RAUW with undef.
>
>
>
> Originally, all instructions where RAUW but only non-EHPad were deleted
> (that means EHPad were RAUW but not deleted).
>
> Then it was later patched by not RAUW token instructions and now not
> deleting EHPad nor token instructions.
>
>
>
> My assumption is that the instructions we wanted to avoid RAUW were only
> the EHPad, not all the token instructions.
>
> So I'm changing it into RAUW all non-EHPad instructions and delete all
> non-EHPad instructions.
>
>
>
> But maybe my assumption is incorrect and there are token instructions that
> are non-EHPad that we want to skip too?
>
>
>
> On Tue, Jun 16, 2020 at 7:55 PM Eli Friedman <efriedma at quicinc.com> wrote:
>
> In general, we have to RAUW before we erase an instruction in dead code;
> even if we know the instruction is dead, it could still have uses in other
> dead code.  If an instruction still has uses, we can’t erase it.
>
>
>
> -Eli
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of *Alexandre
> Isoard via llvm-dev
> *Sent:* Tuesday, June 16, 2020 7:33 PM
> *To:* David Majnemer <david.majnemer at gmail.com>; llvm-dev <
> llvm-dev at lists.llvm.org>
> *Subject:* [EXT] [llvm-dev] InstCombine doesn't delete instructions with
> token
>
>
>
> Hello David,
>
>
>
> I am having an issue with some custom intrinsics that return a token
> value: InstCombine deletes the users of the token but not the instruction
> that creates the token itself. The IR is still valid but it's wasted.
>
>
>
> The source of the issue is coming from an old patch of yours:
>
>
>
> commit 7204cff0a121ebc770cf81f7f94679ae7324daae
> Author: David Majnemer <david.majnemer at gmail.com>
> Date:   Fri Nov 6 21:26:32 2015 +0000
>
>     [InstCombine] Don't RAUW tokens with undef
>
>     Let SimplifyCFG remove unreachable BBs which define token instructions.
>
>     llvm-svn: 252343
>
> --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
> +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
> @@ -3012,7 +3012,7 @@ static bool prepareICWorklistFromFunction(Function
> &F, const DataLayout &DL,
>      while (EndInst != BB->begin()) {
>        // Delete the next to last instruction.
>        Instruction *Inst = &*--EndInst->getIterator();
> -      if (!Inst->use_empty())
> +      if (!Inst->use_empty() && !Inst->getType()->isTokenTy())
>          Inst->replaceAllUsesWith(UndefValue::get(Inst->getType()));
> -      if (Inst->isEHPad()) {
>
> +      if (Inst->isEHPad() || Inst->getType()->isTokenTy()) {
>          EndInst = Inst;
> @@ -3022,7 +3022,8 @@ static bool prepareICWorklistFromFunction(Function
> &F, const DataLayout &DL,
>          ++NumDeadInst;
>          MadeIRChange = true;
>        }
>        Inst->eraseFromParent();
>      }
>    }
>
>
>
> I believe the goal was to avoid RAUW the EHPad (as we don't delete the
> associated EHRet) and not skip all of the token instructions. At least you
> only test on EHPad in the associated unit test.
>
>
>
> In which case we could instead do:
>
>
>
>      while (EndInst != BB->begin()) {
>        // Delete the next to last instruction.
>        Instruction *Inst = &*--EndInst->getIterator();
> -      if (!Inst->use_empty())
> +      if (!Inst->use_empty() && !Inst->isEHPad())
>          Inst->replaceAllUsesWith(UndefValue::get(Inst->getType()));
>        if (Inst->isEHPad()) {
>
>          EndInst = Inst;
> @@ -3022,7 +3022,8 @@ static bool prepareICWorklistFromFunction(Function
> &F, const DataLayout &DL,
>          ++NumDeadInst;
>          MadeIRChange = true;
>        }
>        Inst->eraseFromParent();
>      }
>
>
>
> Is my assumption correct?
>
>
>
> Note that the code is now in
> llvm::removeAllNonTerminatorAndEHPadInstructions of
> llvm/lib/Transforms/Utils/Local.cpp
>
>
>
> --
>
> *Alexandre Isoard*
>
>
>
>
> --
>
> *Alexandre Isoard*
>
>
>
>
> --
>
> *Alexandre Isoard*
>


-- 
*Alexandre Isoard*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200617/8742a2ab/attachment.html>