[PATCH] Fix illegal DAG produced by SelectionDAG::getConstant() for v2i64 type
Daniel Sanders
daniel.sanders at imgtec.com
Fri Oct 18 03:03:27 PDT 2013
I've already had some feedback on this patch. For the sake of completeness, here is the thread:
> -----Original Message-----
> From: Benjamin Kramer [mailto:benny.kra at gmail.com]
> Sent: 16 October 2013 17:24
> To: Daniel Sanders
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm] r192795 - DAGCombiner: Don't fold xor into not if
> getNOT would introduce an illegal constant.
>
>
> On 16.10.2013, at 18:04, Daniel Sanders <Daniel.Sanders at imgtec.com>
> wrote:
>
> > Hi,
> >
> > I think I'm working on a patch that's relevant to this. On MIPS32
> > with MSA
> we also have a legal v2i64 and an illegal i64 and I've had similar
> problems with the Legalizer and DAGCombiner introducing illegal types.
> There's a few other triggers such as legalizing v2i64 UNDEF.
> >
> > My patch causes SelectionDAG::getConstant() to produce an
> > appropriate
> bitcasted build_vector when given a legal vector type whose elements
> are normally expanded by the type legalizer.
> >
> > I've just sent the patch series to my colleagues for our internal
> > code
> review. I'll be posting the patches to llvm-commits afterwards but you
> can see the patches at https://dmz-
> portal.mips.com/bugz/show_bug.cgi?id=1237 if you want a sneak preview.
> Patch 5/5 is the most important one.
>
> That definitely looks like it's going in the right direction, thanks.
> Some things worth considering:
>
> * Do we really want to do this for all constants before legalize
> types? It looks like it adds an unnecessary layer of obfuscation at
> this level (optimizer has to look through bitcasts, etc). It also
> wouldn't surprise me if the DAGCombiner just folds it back into an
> illegal constant. Not a big problem, but creates unnecessary churn.
I agree it shouldn't be doing this before legalize types. I'll fix that.
> * What about <2 x i64> when only i16 is legal? I don't think we have
> such a target currently, but your approach would fail and split it
> into <4 x i16> which is wrong.
I might have misunderstood the API documentation which talks about "one step in the expansion to get to the smaller register". I thought that when i32 is also illegal, getTypeToTransformTo() would still return i32 because it's one step closer to a legal type. I expected that further (currently unwritten) expansions to be required to get to <8 x i16>. If it would return i16 then I can rewrite this code into a loop easily enough.
> - Ben
http://llvm-reviews.chandlerc.com/D1973
More information about the llvm-commits
mailing list