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
On Wednesday 18 February 2009 18:49, David Greene wrote:> 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>.I should mention this is x86-64. The data layout for x86-64 doesn't even mention vector types, so the default from TargetData gets used, which is to set VECTOR_ALIGN to 16 for preferred and ABI alignment: class X86Subtarget : public TargetSubtarget { [...] std::string getDataLayout() const { const char *p; if (is64Bit()) p = "e-p:64:64-s:64-f64:64:64-i64:64:64-f80:128:128"; else { if (isTargetDarwin()) p = "e-p:32:32-f64:32:64-i64:32:64-f80:128:128"; else p = "e-p:32:32-f64:32:64-i64:32:64-f80:32:32"; } return std::string(p); } So maybe the problem is here? -Dave
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
>
On Wednesday 18 February 2009 21:43, Dan Gohman wrote:> 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?Yes, and it fixes the problem. What's your opinion about how TargetData and X86Subtarget define ABI alignment for SSE registers? I think that's suspect too. It's too bad we can't specify separate ABI alignments for v16i/f8, v8i/f16, v4i/f32 and v2i/f64 as we should probably set the ABI alignment to the element alignment. But I guess to be conservative we should set it to 8 bits. Unless I'm misunderstanding the purpose of the ABI alignment. -Dave