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

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp added inline comments.


================
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
----------------
arsenm wrote:
> 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
Sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113448