[llvm] [RISCV][GlobalISel] Legalize G_ADD, G_SUB, G_AND, G_OR, G_XOR on RISC-V Vector Extension (PR #71400)

[Craig Topper via llvm-commits]

topperc wrote:

> > > > Given different conditions
> > > > > nxv1s64, nxv2s64, nxv4s64, nxv8s64 require hasVInstructionsI64 instead of hasVInstructions
> > > > 
> > > > 
> > > > > nxv1s8, nxv1s16, nxv1s32 require Subtarget.getELen() == 64
> > > > 
> > > > 
> > > > I have this literal implementation locally:
> > > > ```c++
> > > > getActionDefinitionsBuilder({G_ADD, G_SUB, G_AND, G_OR, G_XOR})
> > > >         .legalFor({s32, sXLen})
> > > >         .legalFor(ST.hasVInstructions()
> > > >                       ? std::initializer_list<LLT>{nxv2s8, nxv4s8, nxv8s8,
> > > >                                                    nxv16s8, nxv32s8, nxv64s8,
> > > >                                                    nxv2s16, nxv4s16, nxv8s16,
> > > >                                                    nxv16s16, nxv32s16, nxv2s32,
> > > >                                                    nxv4s32, nxv8s32, nxv16s32}
> > > >                       : std::initializer_list<LLT>())
> > > >         .legalFor(
> > > >             ST.hasVInstructionsI64()
> > > >                 ? std::initializer_list<LLT>{nxv1s64, nxv2s64, nxv4s64, nxv8s64}
> > > >                 : std::initializer_list<LLT>())
> > > >         .legalFor(ST.getELen() == 64
> > > >                       ? std::initializer_list<LLT>{nxv1s8, nxv1s16, nxv1s32}
> > > >                       : std::initializer_list<LLT>())
> > > >         .widenScalarToNextPow2(0)
> > > >         .clampScalar(0, s32, sXLen);
> > > > ```
> > > > 
> > > > 
> > > >     
> > > >       
> > > >     
> > > > 
> > > >       
> > > >     
> > > > 
> > > >     
> > > >   
> > > > Please point out any issue this code snippet has.
> > > 
> > > 
> > > This LGTM.
> > > > In addition, it has to have fragments of `AllVecTys`, so I was wondering, as per @michaelmaitland 's review:
> > > > > In any case, I'd like to have AllVecTys exist so it can be reused by future patches.
> > > > 
> > > > 
> > > > Shall we still be declaring `AllVecTys`? It's good to declare it because it's likely to be reused, what if future patches also need the aforementioned condition checking so it won't be reused as a whole, is it still worth it?
> > > 
> > > 
> > > Since we are not using AllVecTys, I think we should not include this variable. The compiler would emit a warning for an unused variable, and we don't want that. I think you should make this change and bring this PR out of draft, marking it ready for review!
> > 
> > 
> > Something like this could work.
> > ```
> > .legalFor([=, &ST](const LegalityQuery &Query) {
> >   return ST.hasVInstruction() && typeInSet(0, AllVecTys)(Query) &&
> >              (Query.Types[0].getScalarSize() != 64 || ST.hasVInstruction64()) &&
> >              (Query.Types[0].getElementCount().getKnownMinValue() != 1 || ST.getELEN() == 64));
> > }
> > ```
> 
> I think I would prefer separate legalFor statements compared to the one proposed in the reply. I don't see why we should have an all or nothing scheme. For example, if hasVInstructions() but not hasVInstructions64(), we should be able to legalize `nxv2s8, nxv4s8, nxv8s8, nxv16s8, nxv32s8, nxv64s8, nxv2s16, nxv4s16, nxv8s16, nxv16s16, nxv32s16, nxv2s32, nxv4s32, nxv8s32, nxv16s32` but not `nxv1s64, nxv2s64, nxv4s64, nxv8s64`. Using a single `legalFor` with AllVecTys prevents that from happening.

How does it prevent that from happening? The rest of the checks are just programmatically excluding the types that aren't legal without their additional predicates? This lambda will eventually need to move into a freestanding function because we  need the same predicate on every vector instruction. We should avoid 3 legalFors on every different instruction.

https://github.com/llvm/llvm-project/pull/71400