Quentin Colombet via llvm-dev
2017-Jun-29 00:13 UTC
[llvm-dev] Ok with mismatch between dead-markings in BUNDLE and bundled instructions?
> On Jun 28, 2017, at 5:10 PM, Quentin Colombet via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Oh wait, vreg1 is indeed used. > Yeah, having a dead flag here sounds wrong.I mean on the instruction itself. On the bundle, that’s debatable. That would fit the semantic “if no side effect you can kill it” (here there is side effect, we define other vregs).> >> On Jun 28, 2017, at 2:28 AM, Björn Pettersson A <bjorn.a.pettersson at ericsson.com> wrote: >> >> Not sure if I could follow everything in this discussion regarding subregisters. But I think the problem posted by Mikael just happened to involve subregisters, and the discussions about subregisters is confusing when it comes to Mikaels original question/problem. >> >> I think that the bundle could look something like this just as well: >> >> BUNDLE %vreg1<def,dead> >> * %vreg1<def> = add %vreg2, %vreg3 >> * call @foo, %vreg1<internal-use> >> >> No subregisters involved. >> %vreg1 is dead after the bundle. >> %vreg1 is not dead when defined at the "add", because it is used later in the same bundle. >> >> Should perhaps the %vreg1 not be included in the BUNDLE head at all here? >> (but shouldn't the BUNDLE head be a summary of what is going on inside the bundle, so leaving out information about %vreg1 being defined seems wrong) >> >> To me it seems wrong to add "dead" to the def of %vreg1 at the add (considering the internal-use). >> Maybe that even answers the question that the "mismatch" between dead-markings should be OK. >> Or would it be OK to mark %vreg1 as dead at the add, even though we have a later internal-use? >> >> Regards, >> Björn >> >>> -----Original Message----- >>> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of >>> Quentin Colombet via llvm-dev >>> Sent: den 28 juni 2017 00:02 >>> To: Matthias Braun <mbraun at apple.com> >>> Cc: llvm-dev at lists.llvm.org >>> Subject: Re: [llvm-dev] Ok with mismatch between dead-markings in >>> BUNDLE and bundled instructions? >>> >>> >>>> On Jun 27, 2017, at 2:51 PM, Matthias Braun via llvm-dev <llvm- >>> dev at lists.llvm.org> wrote: >>>> >>>> >>>>> On Jun 27, 2017, at 2:44 PM, Krzysztof Parzyszek via llvm-dev <llvm- >>> dev at lists.llvm.org> wrote: >>>>> >>>>> On 6/27/2017 4:35 PM, Quentin Colombet via llvm-dev wrote: >>>>>> Yeah I was reading this as “only the non-touched part are dead”, and >>> that’s what I’d like to see in the representation longer. Obviously, the >>> register is not dead as a whole here :) >>>>> >>>>> I think that having two defs for the same register, one dead and one not >>> dead simply doesn't make sense. We already assume that a register is live if >>> at least a part of it is live, so if it's "dead", it should mean that the whole thing >>> is dead. >>>> Without subregister I would agree. However with subregisters and aliases >>> in play you can express more situations. Like for example: >>>> >>>> %rax<dead>, %eax = ... >>>> >>>> could mean the instruction writes the full rax register but we are only >>> gonna read eax later. >>> >>> That sounds like an alias to: >>> %rax<def-undef, subeax> = … >>> >>> Which sounds fine. Though I am not suggesting we want to move to this >>> dead model for such situation. >>> >>>> That said I am not sure whether we actually need it, and if llvm works that >>> way today. Given how subtle all of this is there is also a high danger that we >>> won't get the bahviour consistent. >>> >>> I agree that consistent behavior is important and I also think we probably >>> cannot model what we want with the current representation. What I would >>> like to see if that we don’t sit on potentially useful information, like this part >>> of the register is dead, because it is convenient implementation-wise. I am >>> not saying that’s what you're suggesting! >>> I agree that at the end of the day we want something that works and that is >>> understandable. To me having the semantic of dead being this can be killed if >>> the instruction does not have side effects sounded easy enough to >>> understand. >>> >>> What is your proposal for the semantic? >>> >>> (IIRC the dead flag is required for values that are never used and the >>> proposed fix somehow goes against that.) >>> >>>> >>>> - Matthias >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> llvm-dev at lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Mikael Holmén via llvm-dev
2017-Jul-03 11:13 UTC
[llvm-dev] Ok with mismatch between dead-markings in BUNDLE and bundled instructions?
Hi, Ok, so to solve the problem for our out-of-tree target we currently do: ------------------------ lib/CodeGen/InlineSpiller.cpp ------------------------ index 67c7e506add..28245a49477 100644 @@ -987,38 +987,46 @@ void InlineSpiller::spillAroundUses(unsigned Reg) { if (foldMemoryOperand(Ops)) continue; // Create a new virtual register for spill/fill. // FIXME: Infer regclass from instruction alone. unsigned NewVReg = Edit->createFrom(Reg); if (RI.Reads) insertReload(NewVReg, Idx, MI); // Rewrite instruction operands. bool hasLiveDef = false; for (const auto &OpPair : Ops) { MachineOperand &MO = OpPair.first->getOperand(OpPair.second); MO.setReg(NewVReg); if (MO.isUse()) { if (!OpPair.first->isRegTiedToDefOperand(OpPair.second)) MO.setIsKill(); } else { + // For bundled instructions: Only examine defs in the BUNDLE + // instruction itself if it exists. + MachineInstr *DefMI = OpPair.first; + if (DefMI->isBundled() && + getBundleStart(DefMI->getIterator())->isBundle() && + !DefMI->isBundle()) + continue; + if (!MO.isDead()) hasLiveDef = true; } } DEBUG(dbgs() << "\trewrite: " << Idx << '\t' << *MI << '\n'); Is there any point of me putting this patch in Phabricator? I have very little hope of managing to trigger the problem on any in-tree target. Regards, Mikael On 06/29/2017 02:13 AM, Quentin Colombet via llvm-dev wrote:> >> On Jun 28, 2017, at 5:10 PM, Quentin Colombet via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> Oh wait, vreg1 is indeed used. >> Yeah, having a dead flag here sounds wrong. > > I mean on the instruction itself. > On the bundle, that’s debatable. That would fit the semantic “if no side effect you can kill it” (here there is side effect, we define other vregs). > >> >>> On Jun 28, 2017, at 2:28 AM, Björn Pettersson A <bjorn.a.pettersson at ericsson.com> wrote: >>> >>> Not sure if I could follow everything in this discussion regarding subregisters. But I think the problem posted by Mikael just happened to involve subregisters, and the discussions about subregisters is confusing when it comes to Mikaels original question/problem. >>> >>> I think that the bundle could look something like this just as well: >>> >>> BUNDLE %vreg1<def,dead> >>> * %vreg1<def> = add %vreg2, %vreg3 >>> * call @foo, %vreg1<internal-use> >>> >>> No subregisters involved. >>> %vreg1 is dead after the bundle. >>> %vreg1 is not dead when defined at the "add", because it is used later in the same bundle. >>> >>> Should perhaps the %vreg1 not be included in the BUNDLE head at all here? >>> (but shouldn't the BUNDLE head be a summary of what is going on inside the bundle, so leaving out information about %vreg1 being defined seems wrong) >>> >>> To me it seems wrong to add "dead" to the def of %vreg1 at the add (considering the internal-use). >>> Maybe that even answers the question that the "mismatch" between dead-markings should be OK. >>> Or would it be OK to mark %vreg1 as dead at the add, even though we have a later internal-use? >>> >>> Regards, >>> Björn >>> >>>> -----Original Message----- >>>> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of >>>> Quentin Colombet via llvm-dev >>>> Sent: den 28 juni 2017 00:02 >>>> To: Matthias Braun <mbraun at apple.com> >>>> Cc: llvm-dev at lists.llvm.org >>>> Subject: Re: [llvm-dev] Ok with mismatch between dead-markings in >>>> BUNDLE and bundled instructions? >>>> >>>> >>>>> On Jun 27, 2017, at 2:51 PM, Matthias Braun via llvm-dev <llvm- >>>> dev at lists.llvm.org> wrote: >>>>> >>>>> >>>>>> On Jun 27, 2017, at 2:44 PM, Krzysztof Parzyszek via llvm-dev <llvm- >>>> dev at lists.llvm.org> wrote: >>>>>> >>>>>> On 6/27/2017 4:35 PM, Quentin Colombet via llvm-dev wrote: >>>>>>> Yeah I was reading this as “only the non-touched part are dead”, and >>>> that’s what I’d like to see in the representation longer. Obviously, the >>>> register is not dead as a whole here :) >>>>>> >>>>>> I think that having two defs for the same register, one dead and one not >>>> dead simply doesn't make sense. We already assume that a register is live if >>>> at least a part of it is live, so if it's "dead", it should mean that the whole thing >>>> is dead. >>>>> Without subregister I would agree. However with subregisters and aliases >>>> in play you can express more situations. Like for example: >>>>> >>>>> %rax<dead>, %eax = ... >>>>> >>>>> could mean the instruction writes the full rax register but we are only >>>> gonna read eax later. >>>> >>>> That sounds like an alias to: >>>> %rax<def-undef, subeax> = … >>>> >>>> Which sounds fine. Though I am not suggesting we want to move to this >>>> dead model for such situation. >>>> >>>>> That said I am not sure whether we actually need it, and if llvm works that >>>> way today. Given how subtle all of this is there is also a high danger that we >>>> won't get the bahviour consistent. >>>> >>>> I agree that consistent behavior is important and I also think we probably >>>> cannot model what we want with the current representation. What I would >>>> like to see if that we don’t sit on potentially useful information, like this part >>>> of the register is dead, because it is convenient implementation-wise. I am >>>> not saying that’s what you're suggesting! >>>> I agree that at the end of the day we want something that works and that is >>>> understandable. To me having the semantic of dead being this can be killed if >>>> the instruction does not have side effects sounded easy enough to >>>> understand. >>>> >>>> What is your proposal for the semantic? >>>> >>>> (IIRC the dead flag is required for values that are never used and the >>>> proposed fix somehow goes against that.) >>>> >>>>> >>>>> - Matthias >>>>> _______________________________________________ >>>>> LLVM Developers mailing list >>>>> llvm-dev at lists.llvm.org >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> llvm-dev at lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
Quentin Colombet via llvm-dev
2017-Jul-05 17:02 UTC
[llvm-dev] Ok with mismatch between dead-markings in BUNDLE and bundled instructions?
> On Jul 3, 2017, at 4:13 AM, Mikael Holmén <mikael.holmen at ericsson.com> wrote: > > Hi, > > Ok, so to solve the problem for our out-of-tree target we currently do: > > ------------------------ lib/CodeGen/InlineSpiller.cpp ------------------------ > index 67c7e506add..28245a49477 100644 > @@ -987,38 +987,46 @@ void InlineSpiller::spillAroundUses(unsigned Reg) { > if (foldMemoryOperand(Ops)) > continue; > > // Create a new virtual register for spill/fill. > // FIXME: Infer regclass from instruction alone. > unsigned NewVReg = Edit->createFrom(Reg); > > if (RI.Reads) > insertReload(NewVReg, Idx, MI); > > // Rewrite instruction operands. > bool hasLiveDef = false; > for (const auto &OpPair : Ops) { > MachineOperand &MO = OpPair.first->getOperand(OpPair.second); > MO.setReg(NewVReg); > if (MO.isUse()) { > if (!OpPair.first->isRegTiedToDefOperand(OpPair.second)) > MO.setIsKill(); > } else { > + // For bundled instructions: Only examine defs in the BUNDLE > + // instruction itself if it exists. > + MachineInstr *DefMI = OpPair.first; > + if (DefMI->isBundled() && > + getBundleStart(DefMI->getIterator())->isBundle() && > + !DefMI->isBundle()) > + continue; > + > if (!MO.isDead()) > hasLiveDef = true; > } > } > DEBUG(dbgs() << "\trewrite: " << Idx << '\t' << *MI << '\n'); > > > Is there any point of me putting this patch in Phabricator? I have very little hope of managing to trigger the problem on any in-tree target.You could use a .mir file, with NOOPs that clobber a lot of regs and fake bundles here and there.> > Regards, > Mikael > > > On 06/29/2017 02:13 AM, Quentin Colombet via llvm-dev wrote: >>> On Jun 28, 2017, at 5:10 PM, Quentin Colombet via llvm-dev <llvm-dev at lists.llvm.org> wrote: >>> >>> Oh wait, vreg1 is indeed used. >>> Yeah, having a dead flag here sounds wrong. >> I mean on the instruction itself. >> On the bundle, that’s debatable. That would fit the semantic “if no side effect you can kill it” (here there is side effect, we define other vregs). >>> >>>> On Jun 28, 2017, at 2:28 AM, Björn Pettersson A <bjorn.a.pettersson at ericsson.com> wrote: >>>> >>>> Not sure if I could follow everything in this discussion regarding subregisters. But I think the problem posted by Mikael just happened to involve subregisters, and the discussions about subregisters is confusing when it comes to Mikaels original question/problem. >>>> >>>> I think that the bundle could look something like this just as well: >>>> >>>> BUNDLE %vreg1<def,dead> >>>> * %vreg1<def> = add %vreg2, %vreg3 >>>> * call @foo, %vreg1<internal-use> >>>> >>>> No subregisters involved. >>>> %vreg1 is dead after the bundle. >>>> %vreg1 is not dead when defined at the "add", because it is used later in the same bundle. >>>> >>>> Should perhaps the %vreg1 not be included in the BUNDLE head at all here? >>>> (but shouldn't the BUNDLE head be a summary of what is going on inside the bundle, so leaving out information about %vreg1 being defined seems wrong) >>>> >>>> To me it seems wrong to add "dead" to the def of %vreg1 at the add (considering the internal-use). >>>> Maybe that even answers the question that the "mismatch" between dead-markings should be OK. >>>> Or would it be OK to mark %vreg1 as dead at the add, even though we have a later internal-use? >>>> >>>> Regards, >>>> Björn >>>> >>>>> -----Original Message----- >>>>> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of >>>>> Quentin Colombet via llvm-dev >>>>> Sent: den 28 juni 2017 00:02 >>>>> To: Matthias Braun <mbraun at apple.com> >>>>> Cc: llvm-dev at lists.llvm.org >>>>> Subject: Re: [llvm-dev] Ok with mismatch between dead-markings in >>>>> BUNDLE and bundled instructions? >>>>> >>>>> >>>>>> On Jun 27, 2017, at 2:51 PM, Matthias Braun via llvm-dev <llvm- >>>>> dev at lists.llvm.org> wrote: >>>>>> >>>>>> >>>>>>> On Jun 27, 2017, at 2:44 PM, Krzysztof Parzyszek via llvm-dev <llvm- >>>>> dev at lists.llvm.org> wrote: >>>>>>> >>>>>>> On 6/27/2017 4:35 PM, Quentin Colombet via llvm-dev wrote: >>>>>>>> Yeah I was reading this as “only the non-touched part are dead”, and >>>>> that’s what I’d like to see in the representation longer. Obviously, the >>>>> register is not dead as a whole here :) >>>>>>> >>>>>>> I think that having two defs for the same register, one dead and one not >>>>> dead simply doesn't make sense. We already assume that a register is live if >>>>> at least a part of it is live, so if it's "dead", it should mean that the whole thing >>>>> is dead. >>>>>> Without subregister I would agree. However with subregisters and aliases >>>>> in play you can express more situations. Like for example: >>>>>> >>>>>> %rax<dead>, %eax = ... >>>>>> >>>>>> could mean the instruction writes the full rax register but we are only >>>>> gonna read eax later. >>>>> >>>>> That sounds like an alias to: >>>>> %rax<def-undef, subeax> = … >>>>> >>>>> Which sounds fine. Though I am not suggesting we want to move to this >>>>> dead model for such situation. >>>>> >>>>>> That said I am not sure whether we actually need it, and if llvm works that >>>>> way today. Given how subtle all of this is there is also a high danger that we >>>>> won't get the bahviour consistent. >>>>> >>>>> I agree that consistent behavior is important and I also think we probably >>>>> cannot model what we want with the current representation. What I would >>>>> like to see if that we don’t sit on potentially useful information, like this part >>>>> of the register is dead, because it is convenient implementation-wise. I am >>>>> not saying that’s what you're suggesting! >>>>> I agree that at the end of the day we want something that works and that is >>>>> understandable. To me having the semantic of dead being this can be killed if >>>>> the instruction does not have side effects sounded easy enough to >>>>> understand. >>>>> >>>>> What is your proposal for the semantic? >>>>> >>>>> (IIRC the dead flag is required for values that are never used and the >>>>> proposed fix somehow goes against that.) >>>>> >>>>>> >>>>>> - Matthias >>>>>> _______________________________________________ >>>>>> LLVM Developers mailing list >>>>>> llvm-dev at lists.llvm.org >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>> >>>>> _______________________________________________ >>>>> LLVM Developers mailing list >>>>> llvm-dev at lists.llvm.org >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >