[llvm-commits] [llvm] r139407 - in /llvm/trunk: lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/CodeGen/SelectionDAG/SelectionDAG.cpp test/CodeGen/ARM/2011-09-09-OddVectorDivision.ll

[James Molloy]

Hi Duncan,

Thanks for your reply. I see that where I was going wrong was I was thinking the
DAGCombiner had a precondition of receiving a legal DAG, and LegalizeTypes was
responsible for "cleaning up" any illegalalities the DAGCombiner made during its
pass.

I realise now as you're explained it, that the DAGCombiner itself has a
postcondition on generating legal types only (when in strict mode), so it's not
LegalizeTypes' responsibility.

Thanks for the clarification, and apologies for extending this thread so long!

James

> -----Original Message-----
> From: Duncan Sands [mailto:baldrick at free.fr]
> Sent: 12 September 2011 12:31
> To: James Molloy
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [llvm] r139407 - in /llvm/trunk:
> lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> lib/CodeGen/SelectionDAG/SelectionDAG.cpp test/CodeGen/ARM/2011-09-09-
> OddVectorDivision.ll
> 
> Hi James,
> 
> > Please bear with me, because I'm trying to unravel and learn the design
> of SelectionDAG at the same time as querying this fix.
> >
> > If I've read the code right, the sequence goes something like:
> >
> > 1) Run DAGCombine in unrestricted mode.
> > 2) Run LegalizeTypes(): postcondition: "[the DAG] only uses operations
> and types that the target supports".
> > 3) If LegalizeTypes() changed the DAG, run DAGCombine in strictly-legal
> mode again.
> > 4) Run LegalizeVectors(); if this changed the DAG, run LegalizeTypes then
> DAGCombine again, again in strict mode.
> > 5) Legalize()
> > 6) DAGCombine again, strict mode.
> >
> > So by my understanding, before Eli's patch, DAGCombine emitted a DAG
> containing an illegal type as an operand of a BUILD_VECTOR.
> > LegalizeTypes() did not legalize that operand (by design or fault), so
> the output of LegalizeTypes() is illegal and seemingly does not adhere to
> its postcondition in SelectionDAGIsel.cpp.
> 
> I don't know where you got the idea that LegalizeTypes doesn't type
> legalize
> BUILD_VECTOR nodes.  It does.  I didn't follow this thread from the
> beginning,
> but most likely one of the DAGCombiner runs after LegalizeTypes introduced
> a
> BUILD_VECTOR with an illegal type (a bug in DAGCombiner; there have been a
> lot
> of bugs like this in the DAGCombiner in the past).
> 
> Ciao, Duncan.
> 
> >
> > I see the problem as in LegalizeTypes(), not in DAGCombine. What am I
> missing here? (apart from a good knowledge of the target independent
> codegen!)
> >
> > Cheers for any help,
> >
> > James
> > ________________________________________
> > From: llvm-commits-bounces at cs.uiuc.edu [llvm-commits-bounces at cs.uiuc.edu]
> On Behalf Of Duncan Sands [baldrick at free.fr]
> > Sent: 10 September 2011 12:43
> > To: llvm-commits at cs.uiuc.edu
> > Subject: Re: [llvm-commits] [llvm] r139407 - in /llvm/trunk:
> lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> lib/CodeGen/SelectionDAG/SelectionDAG.cpp test/CodeGen/ARM/2011-09-09-
> OddVectorDivision.ll
> >
> > Hi James,
> >
> >> So wouldn't a better solution be to make the TypeLegalizer actually
> legalize the types of BUILD_VECTOR?
> >
> > it does.  LegalizeVectorOps is not part of the type legalizer.
> >
> > Ciao, Duncan.
> >
> >>
> >>>  From LegalizeVectorOps.cpp:
> >>
> >> // This does not legalize vector manipulations like ISD::BUILD_VECTOR,
> >> // or operations that happen to take a vector which are custom-lowered;
> >> // the legalization for such operations never produces nodes
> >> // with illegal types, so it's okay to put off legalizing them until
> >> // SelectionDAG::Legalize runs.
> >>
> >> It seems to me that this statement is false - BUILD_VECTOR can contain
> illegal types after DAGCombine, so it is *not* safe to put off legalizing
> them.
> >>
> >> Unless the legalization should be done in SelectionDAG::Legalize...?:
> >>
> >>     case ISD::BUILD_VECTOR:
> >>       // A weird case: legalization for BUILD_VECTOR never legalizes the
> >>       // operands!
> >>       // FIXME: This really sucks... changing it isn't semantically
> incorrect,
> >>       // but it massively pessimizes the code for floating-point
> BUILD_VECTORs
> >>       // because ConstantFP operands get legalized into constant pool
> loads
> >>       // before the BUILD_VECTOR code can see them.  It doesn't usually
> bite,
> >>       // though, because BUILD_VECTORS usually get lowered into other
> nodes
> >>       // which get legalized properly.
> >>       SimpleFinishLegalizing = false;
> >>       break;
> >>
> >> It seems to me that there is code elsewhere outside of the DAGCombiner
> that is not sticking to its pre or postconditions, so fixing the
> DAGCombiner to produce legal types isn't the best solution. I had
> originally thought that the type legalization was only done before
> DAGCombine, which is why I proposed my original solution (which was similar
> to yours).
> >>
> >> Am I missing something here? There could be other places in the
> DAGCombiner that produces illegal types that aren't legalized that we
> haven't caught yet...
> >>
> >> Cheers,
> >>
> >> James
> >>
> >>
> >> ________________________________________
> >> From: Eli Friedman [eli.friedman at gmail.com]
> >> Sent: 10 September 2011 11:46
> >> To: James Molloy
> >> Cc: llvm-commits at cs.uiuc.edu
> >> Subject: Re: [llvm-commits] [llvm] r139407 - in /llvm/trunk:
> lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> lib/CodeGen/SelectionDAG/SelectionDAG.cpp test/CodeGen/ARM/2011-09-09-
> OddVectorDivision.ll
> >>
> >> On Sat, Sep 10, 2011 at 1:27 AM, James Molloy<James.Molloy at arm.com>
> wrote:
> >>> Hi Eli,
> >>>
> >>> This makes sense, thanks. A related question though, and please excuse
> my ignorance because the documentation of SelectionDAG is incomplete at
> best - under what situations could the DAGCombiner run before the
> DAGTypeLegalizer? My stepping with GDB found the opposite.
> >>
> >> The DAGCombiner always runs before the type legalizer... and after the
> >> type legalizer... and after the type legalizer runs again... see
> >> SelectionDAGISel::CodeGenAndEmitDAG().
> >>
> >> -Eli
> >>
> >>
> >> -- IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium.  Thank you.
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
> > -- IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium.  Thank you.
> >
>