[PATCH] D74590: [LegalizeTypes] Scalarize non-byte sized loads in WidenRecRes_Load and SplitVecResLoad

Fri Feb 14 15:52:46 PST 2020

efriedma added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:3675

+  // A vector must always be stored in memory as-is, i.e. without any padding
+  // between the elements, since various code depend on it, e.g. in the
----------------
craig.topper wrote:
> efriedma wrote:
> > craig.topper wrote:
> > > This is duplicated from the code in TargetLowering because I had to produce a build vector with more results than the original type.
> > > 
> > > Given that the code in TargetLowering is separated in its own pass inside scalarizeVectorLoad, I wonder if I should just have handle both with a helper function local to LegalizeTypes and not touch scalarizeVectorLoad?
> > Can't you just call scalarizeVectorLoad, then widen the result?
> > 
> > I'd prefer to have one copy of the "load a non-byte-sized vector" code... we already have at least one other version in VectorLegalizer::ExpandLoad .  scalarizeVectorLoad seems like a fine place to keep the code in question.
> I tried an INSERT_SUBVECTOR with undef to widen it, but that failed for v3i1->v4i1 because splitvecres_insert_subvector got called and tried to create a store of v3i1. Suggestions for other ways to widen it?
> 
> This code isn't safe after legalize types, it creates an integer of the full vector size to do the load. That might need its own type legalization. So I'm not sure it makes sense in TargetLowering.
Can you just ReplaceValueWith the load with the build_vector, and return `SDValue()`?  Then legalization should kick in again to widen the build_vector, I think.

scalarizeVectorLoad can make integers with illegal types even without your patch; for example, if you scalarizeVectorLoad a v2i64 load on a 32-bit target.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74590