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

[Arnold Schwaighofer]

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