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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93755