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

[Eli Friedman via Phabricator via llvm-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93755