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

Wed Mar 27 06:25:20 PDT 2019

Roman,

I do see llvm-commits in the list of subscribers. 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