[llvm-commits] PATCH: Fix for possible bug in DAGCombiner: visitSTORE

[Duncan Sands]

Hi Tom,

On 06/12/12 17:45, Tom Stellard wrote:
> On Thu, Dec 06, 2012 at 05:20:40PM +0100, Duncan Sands wrote:
>> Hi Tom,
>>
>>> The DAGCombiner was generating some illegal stores (f32 stores, which
>>> I have marked as custom) for the R600 backend,
>>
>> is the f32 type legal?
>>
>
> The f32 type is legal, but I'm trying to make f32 stores illegal in order
> to simplify the patterns in tablegen.  I've marked f32 stores as custom and
> I'm lowering them like this:
>
> store (f32 value), ptr -> store (i32 bitcast (f32 value)), ptr

if you mark them custom then most places (like here) will think it's OK to
produce them.  I think you should just mark it illegal, and enhance the logic
in LegalizeDAG.cpp to do the above transform in this case (logic there tends to
be implemented on an "as needed" basis, so if it's not don't already that does
not mean it shouldn't be added).

> I can live without this conversion, so it's OK if I can't get it to work, but
> when I looked at the code it seemed wrong to me.

What seems wrong to you about it?

Best wishes, Duncan.

>
>>   and I uncovered what I
>>> think is a bug in the visitSTORE function.  I have attached a patch that
>>> fixes the bug on R600, but it causes 4 X86 tests to fail:
>>>
>>> LLVM :: CodeGen/X86/2008-06-13-NotVolatileLoadStore.ll
>>> LLVM :: CodeGen/X86/2008-06-13-VolatileLoadStore.ll
>>> LLVM :: CodeGen/X86/mmx-arg-passing.ll
>>> LLVM :: CodeGen/X86/vec_extract-sse4.ll
>>>
>>> You were the last person to modify this piece of code (in 2008!), so I
>>> was wondering if you could take a look.
>>
>> I even vaguely recall this!
>>
>>> diff --git lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> index 5416a5c..26a08ea 100644
>>> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> @@ -7916,9 +7916,8 @@ SDValue DAGCombiner::visitSTORE(SDNode *N) {
>>>      EVT SVT = Value.getOperand(0).getValueType();
>>>      unsigned Align = TLI.getDataLayout()->
>>>        getABITypeAlignment(SVT.getTypeForEVT(*DAG.getContext()));
>>> -    if (Align <= OrigAlign &&
>>> -        ((!LegalOperations && !ST->isVolatile()) ||
>>> -         TLI.isOperationLegalOrCustom(ISD::STORE, SVT)))
>>> +    if (Align <= OrigAlign && !ST->isVolatile()  &&
>>> +        !LegalOperations && TLI.isOperationLegalOrCustom(ISD::STORE, SVT))
>>
>> It doesn't make any sense to me to have
>>    !LegalOperations && TLI.isOperationLegalOrCustom(ISD::STORE, SVT)
>> it only makes sense to have
>>    !LegalOperations || TLI.isOperationLegalOrCustom(ISD::STORE, SVT)
>> If LegalOperations is false then that means that you are allowed to introduce
>> illegal operations, so there is no point in checking the legality of the
>> operation.  If LegalOperations is true then with your logic you won't do this
>> transform even if the operation is legal.
>>
>
> I guess I don't quite understand the meaning of TLI.isOperationLegalOrCustom().
> If this function returns true, does this mean the operation is considered legal?
> For operations marked custom, will the custom lowering hook ever be called during or
> after the final DAG optimization pass?
>
>
>> It would help if you could provide a testcase (you need one with the patch
>> anyway) and explain exactly what goes wrong.
>>
>
> I have attached a test case from the R600 backend, which is currently
> an out of tree target, so I'm not sure if it really helps.
>
> -Tom
>
>
>> Best wishes, Duncan.
>>
>>>        return DAG.getStore(Chain, N->getDebugLoc(), Value.getOperand(0),
>>>                            Ptr, ST->getPointerInfo(), ST->isVolatile(),
>>>                            ST->isNonTemporal(), OrigAlign);
>>
>>
>>