[LLVMdev] Possible DAGCombiner or TargetData Bug

[Dan Gohman]

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