[llvm-dev] InstCombine doesn't delete instructions with token
Alexandre Isoard via llvm-dev
llvm-dev at lists.llvm.org
Wed Jun 17 13:00:52 PDT 2020
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*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200617/dcd195d2/attachment.html>
More information about the llvm-dev
mailing list