[PATCH] Loop Vectorizer: Refactor code to compute vectorized memory instruction cost

[Arnold Schwaighofer]

This is sort of what I am implementing. I still use inheritance for readability, but no virtual functions so that we don't need vtables.


class MemOpCost {
  unsigned Alignment;
  ...
  
  unsigned cost() {
  }

public:

  static unsigned computeCost(Value *MemInst) {
     if (StoreInst *SI = dyn_cast<StoreInst>(MemInst))
         return StoreCost(SI, …).cost();
     LoadInst *LI = cast<LoadInst>(MemInst);
    return LoadCost(LI, …).cost();
  }

private:

   class StoreCost : public MemOpCost {
      StoreCost(StoreInst *Inst) : MemOpCost(…) {
         Alignment =  Inst->getAlignment();
         ...
      } 
   };

   class LoadCost : public MemOpCost {
      StoreCost(LoadInst *Inst) : MemOpCost(…) {
         Alignment =  Inst->getAlignment();
         ...
      } 
   };
};

On Feb 4, 2013, at 11:16 AM, Nadav Rotem <nrotem at apple.com> wrote:

> +    if ((Store = dyn_cast<StoreInst>(MemInst)) != 0) {
> 
> if (Store = dyn_cast<StoreInst>(MemInst)) {
> 
> 
> +  unsigned getOpcode() {
> +    if (Store)
> +      return Store->getOpcode();
> +
> +    return Load->getOpcode();
> +  }
> +
> +  unsigned getAlignment() {
> +    if (Store)
> +      return Store->getAlignment();
> +
> +    return Load->getAlignment();
> +  }
> +
> +  unsigned getPointerAddressSpace() {
> +    if (Store)
> +      return Store->getPointerAddressSpace();
> +
> +    return Load->getPointerAddressSpace();
> +  }
> 
> We can initialize all of these values in the constructor using a single IF statement. I think that it would be better than inheritance. 
> 
> 
> Thanks,
> Nadav
> 
> 
> 
> On Feb 4, 2013, at 7:47 AM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:
> 
>> Hi Renato,
>> 
>> thanks for the feedback - really appreciate it!
>> 
>> 
>> On Feb 4, 2013, at 9:22 AM, Renato Golin <renato.golin at linaro.org> wrote:
>>> 1. You seem to be cramming load & stores with a lot of non-shared code. Maybe it'd be more efficient to have a MemoryCostComp that both LoadCostComp and StoreCostComp derive and implement the specific parts.
>> 
>> I was only going half the way ;). Good idea - will do.
>> 
>> 
>>> 2. Assert Liberally!
>>> 
>>> +    // It better be a load now.
>>> +    Load = cast<LoadInst>(MemInst);
>>> 
>>> to 
>>> 
>>> +    Load = dyn_cast<LoadInst>(MemInst);
>>> +    assert(Load && "Invalid memory instruction type");
>>> 
>>> or use llvm_unreachable().
>> 
>> I think cast internally asserts that the type is right and I think I have seen messages on the mailing list saying that an assert is unnecessary (my memory might deceive here, though) in such a case. Anyway, this code should go away after refactoring the patch.
>> 
>> 
>>> 
>>> 3. All this can be omitted if you use Instr* instead of specific LoadInst/StoreInst and if you common up code in a base class:
>>> 
>>> +  unsigned getOpcode() {
>>> +    if (Store)
>>> +      return Store->getOpcode();
>>> +
>>> +    return Load->getOpcode();
>>> +  }
>>> +
>>> +  unsigned getAlignment() {
>>> +    if (Store)
>>> +      return Store->getAlignment();
>>> +
>>> +    return Load->getAlignment();
>>> +  }
>>> +
>>> +  unsigned getPointerAddressSpace() {
>>> +    if (Store)
>>> +      return Store->getPointerAddressSpace();
>>> +
>>> +    return Load->getPointerAddressSpace();
>>> +  }
>> 
>> Yes, this uglyness will go away if i factor code into sub classes. Note, however that you need LoadInst, StoreInst pointers to call getPointerAddressSpace/getAlignment (unfortunately, they do not share a common MemInst superclass). This is not a problem if we factor code into subclasses.
>> 
>> -Arnold
>> 
>