thr3ads.net - llvm dev - [llvm-dev] InstCombine doesn't delete instructions with token [Jun 2020]

If this information is useful, please help other people find it:
Share via:

Alexandre Isoard via llvm-dev

2020-Jun-17 05:24 UTC

[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*
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200616/baae5235/attachment-0001.html>

Eli Friedman via llvm-dev

2020-Jun-17 19:28 UTC

head link

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

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<mailto: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<mailto: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<mailto:david.majnemer at
gmail.com>>; llvm-dev <llvm-dev at lists.llvm.org<mailto: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<mailto: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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200617/ec211e13/attachment.html>

Alexandre Isoard via llvm-dev

2020-Jun-17 20:00 UTC

head link

[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*
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200617/dcd195d2/attachment.html>

Possibly Parallel Threads

Search for more reasonably related threads

llvm dev - Jun 2020 - InstCombine doesn't delete instructions with token

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

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

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

Possibly Parallel Threads