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

[Renato Golin]

Hi Arnold,

I may be wrong, but I think these guys are only ever used internally:

+    unsigned getOpcode() { return Opcode; }
+
+    unsigned getAlignment() { return Alignment; }
+
+    unsigned getPointerAddressSpace() { return AddressSpace; }
+
+    Value *getPointerOperand() { return PointerOperand; }
+
+    Type *getScalarType() { return ScalarTy; }
+
+    Type *getVectorType() { return VectorTy; }

So, they don't need the getters, and you can use them directly in the other
members of the MemoryOpCost class.

cheers,
--renato


On 4 February 2013 19:24, Arnold Schwaighofer <aschwaighofer at apple.com>wrote:

> 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.
>
>
>
>
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130204/3861edab/attachment.html>