[llvm] r357063 - [AMDGPU][MC] Corrected handling of tied src for atomic return MUBUF opcodes

Wed Mar 27 06:31:24 PDT 2019

Oh I see. Thanks! I will revert the commit shortly.

Regards,
Dmitry

-----Original Message-----
From: Roman Lebedev [mailto:lebedev.ri at gmail.com] 
Sent: 27 марта 2019 г. 16:27
To: Preobrazhensky, Dmitry
Cc: llvm-commits at lists.llvm.org
Subject: Re: [llvm] r357063 - [AMDGPU][MC] Corrected handling of tied src for atomic return MUBUF opcodes

On Wed, Mar 27, 2019 at 4:25 PM Preobrazhensky, Dmitry
<Dmitry.Preobrazhensky at amd.com> wrote:
>
> Roman,
>
> I do see llvm-commits in the list of subscribers.
Open https://reviews.llvm.org/D59305
Ctrl+F for "This revision was automatically updated to reflect the
committed changes."
Ctrl+F for "Herald added a subscriber: llvm-commits"
Note that llvm-commits got subscribed after the commit.

> I believe this one was added automatically.
> Should I revert my commit?
>
> Thanks,
> Dmitry
>
> -----Original Message-----
> From: Roman Lebedev [mailto:lebedev.ri at gmail.com]
> Sent: 27 марта 2019 г. 16:21
> To: Preobrazhensky, Dmitry
> Cc: llvm-commits at lists.llvm.org
> Subject: Re: [llvm] r357063 - [AMDGPU][MC] Corrected handling of tied src for atomic return MUBUF opcodes
>
> On Wed, Mar 27, 2019 at 4:12 PM Preobrazhensky, Dmitry
> <Dmitry.Preobrazhensky at amd.com> wrote:
> >
> > The changes are trivial; reviewers had more than a week for review and had no objections.
> There are two ways in LLVM:
> 1.  the change needs review. in that case it can not be committed
> until it is reviewed and accepted.
>      "reviewers had all that time but didn't review" is not an
> argument as per the guidelines.
> 2. the change (in general, not this specifically) is NFC/does not need
> review/is eligible for post-commit review
>     I don't know all that much about AMDGPU, but given that reviewers
> already commented on the review,
>     it doesn't really look like this applies here..
>
> Also, that review did not have llvm-commits subscribed, so general
> public did not even see it before it got committed.
> (please either do manually subscribe it, or at least correctly set the
> repository of the differential.)
>
> > Should I wait till the change is reviewed?
> Especially given the latter issue, yeah.
>
> > Thanks,
> > Dmitry
> Roman.
>
> > -----Original Message-----
> > From: Roman Lebedev [mailto:lebedev.ri at gmail.com]
> > Sent: 27 марта 2019 г. 16:07
> > To: Preobrazhensky, Dmitry
> > Cc: llvm-commits at lists.llvm.org
> > Subject: Re: [llvm] r357063 - [AMDGPU][MC] Corrected handling of tied src for atomic return MUBUF opcodes
> >
> > Doesn't look like this was reviewed?
> >
> > On Wed, Mar 27, 2019 at 4:06 PM Dmitry Preobrazhensky via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> > >
> > > Author: dpreobra
> > > Date: Wed Mar 27 06:07:41 2019
> > > New Revision: 357063
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=357063&view=rev
> > > Log:
> > > [AMDGPU][MC] Corrected handling of tied src for atomic return MUBUF opcodes
> > >
> > > See bug 40917: https://bugs.llvm.org/show_bug.cgi?id=40917
> > >
> > > Reviewers: artem.tamazov, arsenm
> > >
> > > Differential Revision: https://reviews.llvm.org/D59305
> > >
> > > Modified:
> > >     llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
> > >     llvm/trunk/test/MC/AMDGPU/mubuf.s
> > >
> > > Modified: llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
> > > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp?rev=357063&r1=357062&r2=357063&view=diff
> > > ==============================================================================
> > > --- llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp (original)
> > > +++ llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp Wed Mar 27 06:07:41 2019
> > > @@ -4911,13 +4911,19 @@ void AMDGPUAsmParser::cvtMubufImpl(MCIns
> > >    bool HasLdsModifier = false;
> > >    OptionalImmIndexMap OptionalIdx;
> > >    assert(IsAtomicReturn ? IsAtomic : true);
> > > +  unsigned FirstOperandIdx = 1;
> > >
> > > -  for (unsigned i = 1, e = Operands.size(); i != e; ++i) {
> > > +  for (unsigned i = FirstOperandIdx, e = Operands.size(); i != e; ++i) {
> > >      AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);
> > >
> > >      // Add the register arguments
> > >      if (Op.isReg()) {
> > >        Op.addRegOperands(Inst, 1);
> > > +      // Insert a tied src for atomic return dst.
> > > +      // This cannot be postponed as subsequent calls to
> > > +      // addImmOperands rely on correct number of MC operands.
> > > +      if (IsAtomicReturn && i == FirstOperandIdx)
> > > +        Op.addRegOperands(Inst, 1);
> > >        continue;
> > >      }
> > >
> > > @@ -4955,12 +4961,6 @@ void AMDGPUAsmParser::cvtMubufImpl(MCIns
> > >      }
> > >    }
> > >
> > > -  // Copy $vdata_in operand and insert as $vdata for MUBUF_Atomic RTN insns.
> > > -  if (IsAtomicReturn) {
> > > -    MCInst::iterator I = Inst.begin(); // $vdata_in is always at the beginning.
> > > -    Inst.insert(I, *I);
> > > -  }
> > > -
> > >    addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyOffset);
> > >    if (!IsAtomic) { // glc is hard-coded.
> > >      addOptionalImmOperand(Inst, Operands, OptionalIdx, AMDGPUOperand::ImmTyGLC);
> > >
> > > Modified: llvm/trunk/test/MC/AMDGPU/mubuf.s
> > > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/AMDGPU/mubuf.s?rev=357063&r1=357062&r2=357063&view=diff
> > > ==============================================================================
> > > --- llvm/trunk/test/MC/AMDGPU/mubuf.s (original)
> > > +++ llvm/trunk/test/MC/AMDGPU/mubuf.s Wed Mar 27 06:07:41 2019
> > > @@ -711,6 +711,14 @@ buffer_atomic_inc v1, v[2:3], s[8:11], 5
> > >  // SICI: buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc slc ; encoding: [0x04,0x70,0xf0,0xe0,0x02,0x01,0x42,0xb8]
> > >  // VI:   buffer_atomic_inc v1, v[2:3], s[8:11], 56 idxen offen offset:4 glc slc ; encoding: [0x04,0x70,0x2e,0xe1,0x02,0x01,0x02,0xb8]
> > >
> > > +buffer_atomic_add v5, off, s[8:11], 0.5 offset:4095 glc
> > > +// SICI: buffer_atomic_add v5, off, s[8:11], 0.5 offset:4095 glc ; encoding: [0xff,0x4f,0xc8,0xe0,0x00,0x05,0x02,0xf0]
> > > +// VI:   buffer_atomic_add v5, off, s[8:11], 0.5 offset:4095 glc ; encoding: [0xff,0x4f,0x08,0xe1,0x00,0x05,0x02,0xf0]
> > > +
> > > +buffer_atomic_add v5, off, s[8:11], 0.15915494 offset:4095 glc
> > > +// NOSICI: error: invalid operand for instruction
> > > +// VI:   buffer_atomic_add v5, off, s[8:11], 0.15915494 offset:4095 glc ; encoding: [0xff,0x4f,0x08,0xe1,0x00,0x05,0x02,0xf8]
> > > +
> > >  //===----------------------------------------------------------------------===//
> > >  // Lds support
> > >  //===----------------------------------------------------------------------===//
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at lists.llvm.org
> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits