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

[Nadav Rotem]

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