[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