[llvm-commits] [llvm] r46140 - in /llvm/trunk: include/llvm/Target/ lib/CodeGen/SelectionDAG/ lib/Target/ lib/Target/ARM/ lib/Target/Alpha/ lib/Target/CellSPU/ lib/Target/Mips/ lib/Target/PowerPC/ lib/Target/Sparc/ lib/Target/X86/ test/CodeGen/X86/

[Chris Lattner]

On Jan 17, 2008, at 10:03 PM, Duncan Sands wrote:
> Hi Chris,
>> +  LegalizeAction getTruncStoreAction(MVT::ValueType ValVT,
>> +                                     MVT::ValueType MemVT) const {
>> +    assert(ValVT < MVT::LAST_VALUETYPE && MemVT < 32 &&
>
> what is 32?  Did you mean <= LAST_INTEGER_VALUETYPE or something?   
> Now I
> come to notice it, it is bad that you have to use < for  
> LAST_VALUE_TYPE
> but <= for LAST_INTEGER_VALUETYPE and friends...

32 is sizeof(uint64_t)*8 / 2, because each entry takes two bits in a  
uint64_t.

I converted this to symbolic math to make it more obvious, and  
switched to using array_lengthof instead of comparing against  
MVT::LAST_VALUETYPE directly.

>> -  /// isStoreXLegal - Return true if the specified store with  
>> truncation is
>> +  /// isTruncStoreLegal - Return true if the specified store with  
>> truncation is
>>   /// legal on this target.
>> -  bool isStoreXLegal(MVT::ValueType VT) const {
>> -    return getStoreXAction(VT) == Legal || getStoreXAction(VT) ==  
>> Custom;
>> +  bool isTruncStoreLegal(MVT::ValueType ValVT, MVT::ValueType  
>> MemVT) const {
>> +    return getTruncStoreAction(ValVT, MemVT) == Legal ||
>> +           getTruncStoreAction(ValVT, MemVT) == Custom;
>>   }
>
> It would be more friendly to have isStoreXLegal return false if VT  
> or MemVT is
> an extended value type, rather than having it assert in  
> getTruncStoreAction...

This is tricky because I can't just use isTypeLegal here.  PPC for  
example supports a truncstore from i32 to i8 even though i8 isn't  
legal.  When I return to working on apints, I'll figure out what the  
right answer is.

>> +  // If this is an FP_ROUND or TRUNC followed by a store, fold  
>> this into a
>> +  // truncating store.  We can do this even if this is already a  
>> truncstore.
>> +  if ((Value.getOpcode() == ISD::FP_ROUND || Value.getOpcode() ==  
>> ISD::TRUNCATE)
>> +      && TLI.isTypeLegal(Value.getOperand(0).getValueType()) &&
>> +      Value.Val->hasOneUse() && ST->isUnindexed() &&
>> +      TLI.isTruncStoreLegal(Value.getOperand(0).getValueType(),
>> +                            ST->getStoredVT())) {
>
> For example, this will assert if ST->getStoredVT() >= 32, eg an  
> apfloat (!).  Also,
> before legalize this could be done whether all these guys are legal  
> or not I suppose.

It could be done, but isn't profitable and doesn't simplify the code  
necessarily.

>> +      case Expand:
>> +        // Just store the low part.  This may become a non-trunc  
>> store, so make
>> +        // sure to use getTruncStore, not UpdateNodeOperands below.
>> +        ExpandOp(ST->getValue(), Tmp3, Tmp4);
>> +        return DAG.getTruncStore(Tmp1, Tmp3, Tmp2, ST- 
>> >getSrcValue(),
>> +                                 SVOffset, MVT::i8, isVolatile,  
>> Alignment);
>
> This may be wrong for apints, since the expanded value type may be  
> smaller than
> ST->getStoredVT(), eg if you are expanding i64, and ST- 
> >getStoredVT() is i40.  I
> can take care of this.

Ok.  As it turns out, I think this code is unreachable currently.  If  
we made the above dag combine xform happen even for illegal xforms  
before legalize, then this code would be live.

-Chris