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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:941
+  // Fully customized legalization.
+  Optional<LegalizeKind> CustomLK = getCustomTypeConversion(Context, VT);
+  if (CustomLK)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93755