[LLVMdev] Turning on LegalizeTypes by default

[Duncan Sands]

Hi Chris,

> > Once LegalizeTypes has stabilized, I plan to progressively delete type
> > legalization functionality from LegalizeDAG, checking that  
> > LegalizeTypes
> > has the same functionality as I go, and adding it if not.
> 
> This sounds good.  Note that this is somewhat trickier than it seems  
> and may also expose bugs.  The problems that I hit when I was working  
> on legalize types ages ago was that custom lowering ops on some  
> targets generates nodes with illegal types.  One example (that I think  
> was fixed) was that legalizing a uitofp from 32-bit to double turns  
> into a 64-bit sitofp on x87.  Just expect a bit of fallout when  
> removing chunks from legalizedag.

yes, I'm sure there'll be quite a bit of this kind of thing.

> >  PowerPC/vec_spat.ll (@splat_h) - here code got slightly worse.  The
> > problem here is that LegalizeDAG "cheats": it constructs a  
> > BUILD_VECTOR
> > where the type of the operands doesn't match the vector element type  
> > (this
> > is supposed to be illegal), while LegalizeTypes correctly constructs a
> > BUILD_VECTOR where the operand type is equal to the vector element  
> > type,
> > but then the PPC custom code doesn't handle this optimally.
> 
> After looking at this a bit, I don't think this is the real reason.   
> The issue (I think) is a phase ordering issue between legalize types  
> and custom lowering.  In the old legalize case, legalize just custom  
> lowers the build_vector-of-i16 right away.  In the legalize types  
> case, legalize types modifies the DAG quite a bit to eliminate the i16  
> operations, and the PPC backend doesn't match on what it expects.

You are right.  LegalizeTypes does allow custom lowering, in fact it
handles custom lowering in more unified and systematic way than
LegalizeDAG, but it always tests for whether custom lowering is needed
like this:
  if (TLI.getOperationAction(OpCode, IllegalType) == Custom)

So in the case of BUILD_VECTOR with an illegal (i16) operand type,
this becomes TLI.getOperationAction(BUILD_VECTOR, i16) [the vector
type v816 is itself legal].  However targets like to use the vector
type when setting custom operations, for example PowerPC does:

    setOperationAction(ISD::BUILD_VECTOR, MVT::v16i8, Custom);
    setOperationAction(ISD::BUILD_VECTOR, MVT::v8i16, Custom);
    setOperationAction(ISD::BUILD_VECTOR, MVT::v4i32, Custom);
    setOperationAction(ISD::BUILD_VECTOR, MVT::v4f32, Custom);

One solution would be to add:
    setOperationAction(ISD::BUILD_VECTOR, MVT::i16, Custom);

I guess it would also be possible to query the vector type, but
I don't like that much.  What do you think?

> >  Two solutions:
> > decide that in fact
> >  v8i16 = BUILD_VECTOR(i32, i32, ..., i32)
> > is legal and modify LegalizeTypes to take advantage of this
> 
> Are you saying that the build_vector would have 8 i32 inputs, or only  
> 4?  If 8, I really don't like it because it breaks a lot of  
> invariants.

I mean 8.  I don't like it either, yet many places used to build
such vectors (I cleaned them up a few months ago) so it at least
works most of the time!

> If 4 then it should be the same as the current v4i32  
> build vector that we're getting.
> 
> > or improve
> > the PPC so it better handles the code coming out of LegalizeTypes.
> 
> Another option would be to have legalizetypes invoke the ppc custom  
> lowering hook for BUILD_VECTOR when it sees the build_vector of i16.   
> This way, the ppc backend would see the input build_vector unmolested.

See above.

Ciao,

Duncan.