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

Wed Jun 17 13:47:23 PDT 2020

To be more precise, the verifier require all the user of preallocated.arg
to be a preallocated.setup, and requires the def of the
preallocated.setup's argument to be a preallocated.arg.

This allows to delete all users of preallocated.setup (aka. all
preallocated.arg), but not to RAUW (with neither undef nor none).

On Wed, Jun 17, 2020 at 1:40 PM Alexandre Isoard <alexandre.isoard at gmail.com>
wrote:

> 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*
>

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