[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

[Duncan Sands]

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.
>