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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3887
+
+  const MachineInstr *Opnd1Def =
+      getOpcodeDef(TargetOpcode::G_CONSTANT, MI.getOperand(2).getReg(), *MRI);
----------------
abinavpp wrote:
> foad wrote:
> > foad wrote:
> > > Could use getIConstantVRegVal here to get the APInt value directly?
> > `Opnd1` is a bit confusing because it's MI.getOperand(2). Maybe call it something vague like MaskVal?
> For some reason, naming the operands as Val and MaskVal is confusing me, how about LHS and RHS?
Sure.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113448