[PATCH] D93755: [VE][isel] Map EVT vectors to vector registers.

[Simon Moll via Phabricator via llvm-commits]

simoll added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:941
+  // Fully customized legalization.
+  Optional<LegalizeKind> CustomLK = getCustomTypeConversion(Context, VT);
+  if (CustomLK)
----------------
efriedma wrote:
> simoll wrote:
> > simoll wrote:
> > > craig.topper wrote:
> > > > simoll wrote:
> > > > > efriedma wrote:
> > > > > > simoll wrote:
> > > > > > > efriedma wrote:
> > > > > > > > getTypeConversion is pretty closely tied to the conversions we actually support in type legalization.  I'm not sure it's a good idea to let the target completely override the logic here.  If there's some specific aspect of this function you'd like to customize, we can look at adding a more specific hook.
> > > > > > > `getCustomTypeConversion` returns a `LegalizeKind` with limited options for legalization strategies  - we could even type check `LegalizeKind` in its constructor and assert if an illegal/unimplemented legalization step is attempted.
> > > > > > > 
> > > > > > > The fundamental issue with calling back from `getTypeConversion` is that it tries to peg all legalization steps to MVTs. That breaks down as soon as the intermediate step of legalization can only be an EVT. For example:
> > > > > > > 
> > > > > > > `v17i17 -> v17i32 -> v256i32`
> > > > > > I'm not quite sure I understand the issue here.
> > > > > > 
> > > > > > The logic in this function should be doing something like v17i17 -> v32i17 -> v32i32, then doing a lookup in ValueTypeActions, I think?  If that isn't working, there's probably something wrong with the logic here.  Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal?  But if that isn't working, we should probably just fix it for all targets.
> > > > > > 
> > > > > > At the very least, the getCustomTypeConversion() should go after the VT.isSimple() check; there isn't any reason for the ValueTypeActions table to be missing entries.
> > > > > > [..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.
> > > > > 
> > > > > Not quite. `getTypeConversion(v17i17)` promotes the element type to `i17`. Conversion stops here.
> > > > > `getVectorTypeBreakdown` sees that `v17i32` is not a power-of-two vector and decides to scalarize.
> > > > > 
> > > > > > At the very least, the getCustomTypeConversion() should go after the VT.isSimple() check; there isn't any reason for the ValueTypeActions table to be missing entries.
> > > > > 
> > > > > The `ValueTypeActions` table is an implementation detail of `getTypeConversion`. It is intentional that `getCustomTypeConversion` overrides this.
> > > > > > [..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.
> > > > > 
> > > > > Not quite. `getTypeConversion(v17i17)` promotes the element type to `i17`. Conversion stops here.
> > > > > `getVectorTypeBreakdown` sees that `v17i32` is not a power-of-two vector and decides to scalarize.
> > > > >
> > > > 
> > > > I see that as a bug in getVectorTypeBreakdown. X86 recently had to add a hack to getRegisterTypeForCallingConv/getNumRegistersForCallingConv to make v3f16 work correctly to follow the ABI for a struct of 3 halfs.
> > > > > > [..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.
> > > > > 
> > > > > Not quite. `getTypeConversion(v17i17)` promotes the element type to `i17`. Conversion stops here.
> > > > > `getVectorTypeBreakdown` sees that `v17i32` is not a power-of-two vector and decides to scalarize.
> > > > >
> > > > 
> > > > I see that as a bug in getVectorTypeBreakdown. X86 recently had to add a hack to getRegisterTypeForCallingConv/getNumRegistersForCallingConv to make v3f16 work correctly to follow the ABI for a struct of 3 halfs.
> > > 
> > > We have a similar fix here.
> > > `VETargetLowering::getVectorTypeBreakdownForCallingConv` calls `getCustomTypeConversion` in a loop to make parameter passing consistent with type conversion. That way, the problem goes away. eg, `v3f32` becomes `v256f32`
> > > > > [..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.
> > > > 
> > > > Not quite. `getTypeConversion(v17i17)` promotes the element type to `i17`. Conversion stops here.
> > > > `getVectorTypeBreakdown` sees that `v17i32` is not a power-of-two vector and decides to scalarize.
> > > >
> > > 
> > > I see that as a bug in getVectorTypeBreakdown. X86 recently had to add a hack to getRegisterTypeForCallingConv/getNumRegistersForCallingConv to make v3f16 work correctly to follow the ABI for a struct of 3 halfs.
> > 
> > 
> > > At the very least, the getCustomTypeConversion() should go after the VT.isSimple() check; there isn't any reason for the ValueTypeActions table to be missing entries.
> > 
> > The `ValueTypeActions` table is an implementation detail of `getTypeConversion`. It is intentional that `getCustomTypeConversion` overrides this.
> 
> Sort of?  We have target hooks that control the contents of the table; see getPreferredVectorAction() and softPromoteHalfType().
I don't disagree.
But isn't it better to have one concise function that let's you specify exactly what you want, than a bunch of hooks that are all over the place?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93755