[PATCH] D115881: [WIP][AMDGPU][GlobalISel] Add patterns for no-return atomic ops with single address and data in tblgen.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/DSInstructions.td:130
+: DS_Pseudo<opName,
+  (outs data_op:$vdst), // FIXME: Empty (outs) is not working in global-isel.
+  (ins VGPR_32:$addr, getLdStRegisterOperand<rc>.ret:$data0, offset:$offset, gds:$gds),
----------------
abinavpp wrote:
> abinavpp wrote:
> > Petar.Avramovic wrote:
> > > abinavpp wrote:
> > > > This is incorrect, breaking the MC tests and might be related to the change in
> > > > SIInsertWaitcnts behaviour in some of the modified tests. How can we fix this?
> > > // FIXME: Empty (outs) is not working in global-isel.
> > > 
> > > Check GlobalISelEmitter.cpp for:
> > > ```
> > >   if (DstI.Operands.NumDefs < Src->getExtTypes().size())
> > >     return failedImport("Src pattern result has more defs than dst MI (" +
> > >                         to_string(Src->getExtTypes().size()) + " def(s) vs " +
> > >                         to_string(DstI.Operands.NumDefs) + " def(s))");
> > > ```
> > > no ret has unused return in mir right? You want nothing in the output. 
> > Thank you! I didn't know about the -warn-on-skipped-patterns option in
> > global-isel's tblgen back-end.
> > 
> > Along with the modified patterns in this patch, I'm seeing the "Src pattern
> > result has more defs than dst MI" warning for NoRtn `Pat`s like
> > FlatSignedAtomicPatNoRtn, GlobalAtomicNoRtnSaddrPat etc. in FLATInstructions.td
> > without this patch.
> > 
> > In `Pat`s, how do we specify no defs for a src PatFrag to the tblgen back-end
> > when we have a dst `Instruction` with an empty `outs`? The FIXME near class
> > DSAtomicRetPat in this change asks for the same.
> I'm not sure about the right approach here. We're trying to match generic
> atomic ops (excluding store, fence etc. that doesn't return) that are defined
> with return values (atomic SDNode's SDTypeProfile has 1 NumResults,
> G_ATOMICRMW_* have 1 outs) to `Instruction` with an empty outs.  I don't think
> we can define no-return versions for the generic atomics ops since the
> corresponding ones in the IR are return versions. Let me know if I have missed
> anything here.
> 
> P.S. I'll be on vacation this week.
I vaguely recall relaxing this restriction in the DAG. Would it be easier to get this working on the DAG path first, before enabling the gisel selection?

We don't need to change the generic pseudos, the selection predicate should be able to see if the uses are empty. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115881/new/

https://reviews.llvm.org/D115881