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

[Arnold Schwaighofer]

This patch implements the functionality with:

MemoryCostComputation class:
  It provides one static method computeCost that calls the right helper class: StoreCost, LoadCost
  depending on the type of the instruction.

LoadCost, StoreCost:
  Only define constructor that fils fields (Opcode, Alignment, AddressSpace, and so on) in their super class MemoryOpCost.

MemoryOpCost:
  Super class that implements cost estimation functions.

No v-table needed.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Loop-Vectorizer-Refactor-code-to-compute-vectorized-.patch
Type: application/octet-stream
Size: 13703 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130204/8262e90f/attachment.obj>
-------------- next part --------------


Thanks,
Arnold

On Feb 4, 2013, at 11:30 AM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:

> 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
>>> 
>> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits