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
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
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
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.
Cool. I've committed this on trunk now. If you have a reasonably reduced
testcase for this, please add it.
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.
The purpose of ABI alignment is to govern things like struct layouts, global
variables, allocas, and so on. So SSE types on x86 should probably all
remain ABI-aligned at 16 bytes.
I think the particular DAGCombine you pointed out is using ABI alignment
as a conservative heuristic. In some cases it may be safe to transform the
store to a store that doesn't have the ABI alignment for the stored value,
but DAGCombine doesn't know when it's safe. I guess this could be fixed
by having the target provide a third kind of alignment value: the minimum
alignment that the target can store values of a particular type to.
Dan
> Yes, and it fixes the problem.
Cool. I've committed this on trunk now. If you have a reasonably
reduced
testcase for this, please add it.
Working on it. I'm getting our build validated first.
The purpose of ABI alignment is to govern things like struct layouts,
global
variables, allocas, and so on. So SSE types on x86 should probably all
remain ABI-aligned at 16 bytes.
Ok, makes sense.
I think the particular DAGCombine you pointed out is using ABI alignment
as a conservative heuristic. In some cases it may be safe to
transform the
store to a store that doesn't have the ABI alignment for the stored
value,
but DAGCombine doesn't know when it's safe. I guess this could be fixed
by having the target provide a third kind of alignment value: the
minimum
alignment that the target can store values of a particular type to.
Yes, that would provide more information. I'm not sure how critical it is.
My concern is whether someone might think "ABI alignment" is equivalent to
"safe alignment."
If there were a third option, we would somehow want to discriminate based on
vector element type.
-Dave