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

[Tom Stellard]

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

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.
 
>  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);
> 
> 
> 
-------------- next part --------------
;RUN: llc < %s -march=r600 -mcpu=redwood | FileCheck %s

;CHECK: RAT_WRITE_CACHELESS_128 T{{[0-9]+\.XYZW, T[0-9]+\.X}}, 1

define void @test(<4 x float> addrspace(1)* %out, <4 x float> addrspace(1)* %in) {
  %1 = load <4 x float> addrspace(1) * %in
  store <4 x float> %1, <4 x float> addrspace(1)* %out
  ret void
}