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

[Duncan Sands]

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?

  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.

It would help if you could provide a testcase (you need one with the patch
anyway) and explain exactly what goes wrong.

Best wishes, Duncan.

>        return DAG.getStore(Chain, N->getDebugLoc(), Value.getOperand(0),
>                            Ptr, ST->getPointerInfo(), ST->isVolatile(),
>                            ST->isNonTemporal(), OrigAlign);