[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/

[Duncan Sands]

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...


> +  void setTruncStoreAction(MVT::ValueType ValVT, MVT::ValueType MemVT,
> +                           LegalizeAction Action) {
> +    assert(ValVT < MVT::LAST_VALUETYPE && MemVT < 32 && 

Again the magic 32.

> -  /// 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...

> +  // 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.

> +      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.

Ciao,

Duncan.