[LLVMdev] Possible DAGCombiner or TargetData Bug

[Dan Gohman]

I agree, that doesn't look right.  It looks like this
is what was intended:

Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/DAGCombiner.cpp	(revision 65000)
+++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp	(working copy)
@@ -4903,9 +4903,9 @@
   // resultant store does not need a higher alignment than the original.
   if (Value.getOpcode() == ISD::BIT_CONVERT && !ST->isTruncatingStore() &&
       ST->isUnindexed()) {
-    unsigned Align = ST->getAlignment();
+    unsigned OrigAlign = ST->getAlignment();
     MVT SVT = Value.getOperand(0).getValueType();
-    unsigned OrigAlign = TLI.getTargetData()->
+    unsigned Align = TLI.getTargetData()->
       getABITypeAlignment(SVT.getTypeForMVT());
     if (Align <= OrigAlign &&
         ((!LegalOperations && !ST->isVolatile()) ||

Does that look right to you?

Dan

On Wed, February 18, 2009 4:49 pm, David Greene wrote:
> I got bit by this in LLVM 2.4 DagCombiner.cpp and it's still in trunk:
>
> SDValue DAGCombiner::visitSTORE(SDNode *N) {
>
> [...]
>
>   // If this is a store of a bit convert, store the input value if the
>   // resultant store does not need a higher alignment than the original.
>   if (Value.getOpcode() == ISD::BIT_CONVERT && !ST->isTruncatingStore() &&
>       ST->isUnindexed()) {
>     unsigned Align = ST->getAlignment();
>     MVT SVT = Value.getOperand(0).getValueType();
>     unsigned OrigAlign = TLI.getTargetData()->
>       getABITypeAlignment(SVT.getTypeForMVT());
>     if (Align <= OrigAlign &&
>         ((!LegalOperations && !ST->isVolatile()) ||
>          TLI.isOperationLegalOrCustom(ISD::STORE, SVT)))
>       return DAG.getStore(Chain, N->getDebugLoc(), Value.getOperand(0),
>                           Ptr, ST->getSrcValue(),
>                           ST->getSrcValueOffset(), ST->isVolatile(),
> OrigAlign);
>   }
>
> Uhh...this doesn't seem legal to me.  How can we just willy-nilly create a
> store with a greater alignment?  In this case Align is 8 and OrigAlign is
> 16
> because SVT.getTypeForMVT() is Type::VectorTyID (<2 x i64>) which has an
> ABI
> type of VECTOR_ALIGN.
>
> Hmm...why is the ABI alignment for VectorTyID 16?  The ABI certainly
> doesn't
> guarantee it.  It only guarantees it for __int128, __float128 and __m128.
> Lots of other types can map to <2 x i64>.
>
> Any opinions on this?
>
>                                               -Dave
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>