[PATCH] D113448: [AMDGPU] Check for unneeded shift mask in shift PatFrags.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added a comment.
This revision is now accepted and ready to land.

LGTM. Solving constant regbankselect is not really related and shouldn't hold this up



================
Comment at: llvm/test/CodeGen/AMDGPU/constrained-shift.ll:19
+; GISEL-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GISEL-NEXT:    v_and_b32_e32 v1, 15, v1
+; GISEL-NEXT:    v_lshlrev_b16_e32 v2, v1, v0
----------------
abinavpp wrote:
> foad wrote:
> > abinavpp wrote:
> > > abinavpp wrote:
> > > > arsenm wrote:
> > > > > foad wrote:
> > > > > > abinavpp wrote:
> > > > > > > Do we see anything obvious in this change that's not allowing us to eliminate the `and` in global-isel for the divergent cases?
> > > > > > I think a cross-regbank copy is getting in the way of matching the constant value 15. Maybe use getIConstantVRegValWithLookThrough to look through the copy?
> > > > > This is another case where regbankselect or a the post regbank combiner should have materialized the constant in VGPR to begin with
> > > > > I think a cross-regbank copy is getting in the way of matching the constant value 15. Maybe use getIConstantVRegValWithLookThrough to look through the copy?
> > > > 
> > > > getIConstantVRegValWithLookThrough() in the predicate alone won't help here
> > > > since we're not able to match the pattern in the first place.
> > > > This is another case where regbankselect or a the post regbank combiner should have materialized the constant in VGPR to begin with
> > > 
> > > Doing something like:
> > > ```
> > > --- a/llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
> > > +++ b/llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
> > > @@ -472,6 +472,10 @@ RegBankSelect::MappingCost RegBankSelect::computeMapping(
> > >      Register Reg = MO.getReg();
> > >      if (!Reg)
> > >        continue;
> > > +
> > > +    if (MO.isUse() && isConstantOrConstantVector(*(MRI->getVRegDef(Reg)), *MRI))
> > > +      continue;
> > > +
> > >      LLVM_DEBUG(dbgs() << "Opd" << OpIdx << '\n');
> > > ```
> > > or forcing SGPRRegBank for constant operands in
> > > AMDGPURegisterBankInfo::getDefaultMappingVOP() fixes this problem buts ends up
> > > violating the constant bus restriction for a lot of AMDGPU tests.
> > > 
> > > I'm not sure how the original PatFrags (i.e. the ones with the masks as literal
> > > constants without predicates) are working correctly in global-isel for *some*
> > > (vector cases and scalar 64-bit cases are not working) of the divergent cases.
> > > 
> > > Is there a way to write a constant operand in a tblgen DAG that peeks through
> > > trivial cross regbank copies? Or, is there a better way to fix this?
> > Does D113784 help? Anyway see the discussion in that review about how to pick better banks for constants.
> Yes, D113784 will fix this. We can wait till that gets merged.
The solution I decided on for the constant bus problem is we should just not handle it during globalisel at all. VALU mapped instructions should get all VGPR operands. We should have a new and improved SIFoldOperands which would fold SGPRs into instruction operands. The current scheme was built around the assumption that there were attempts to fold before


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113448