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
On Feb 20, 2009, at 3:05 PM, David Greene wrote:> 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.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
On Friday 20 February 2009 17:53, Dan Gohman wrote:> > 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