<div dir="ltr">Hi Arnold,<div><br></div><div style>I may be wrong, but I think these guys are only ever used internally:</div><div style><br></div><div style><div>+ unsigned getOpcode() { return Opcode; }</div><div>+</div>
<div>+ unsigned getAlignment() { return Alignment; }</div><div>+</div><div>+ unsigned getPointerAddressSpace() { return AddressSpace; }</div><div>+</div><div>+ Value *getPointerOperand() { return PointerOperand; }</div>
<div>+</div><div>+ Type *getScalarType() { return ScalarTy; }</div><div>+</div><div>+ Type *getVectorType() { return VectorTy; }</div><div><br></div><div style>So, they don't need the getters, and you can use them directly in the other members of the MemoryOpCost class.</div>
<div style><br></div><div style>cheers,</div><div style>--renato</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 4 February 2013 19:24, Arnold Schwaighofer <span dir="ltr"><<a href="mailto:aschwaighofer@apple.com" target="_blank">aschwaighofer@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This patch implements the functionality with:<br>
<br>
MemoryCostComputation class:<br>
It provides one static method computeCost that calls the right helper class: StoreCost, LoadCost<br>
depending on the type of the instruction.<br>
<br>
LoadCost, StoreCost:<br>
Only define constructor that fils fields (Opcode, Alignment, AddressSpace, and so on) in their super class MemoryOpCost.<br>
<br>
MemoryOpCost:<br>
Super class that implements cost estimation functions.<br>
<br>
No v-table needed.<br>
<br>
<br><br>
<br>
Thanks,<br>
Arnold<br>
<br>
On Feb 4, 2013, at 11:30 AM, Arnold Schwaighofer <<a href="mailto:aschwaighofer@apple.com">aschwaighofer@apple.com</a>> wrote:<br>
<br>
> 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.<br>
><br>
><br>
> class MemOpCost {<br>
> unsigned Alignment;<br>
> ...<br>
><br>
> unsigned cost() {<br>
> }<br>
><br>
> public:<br>
><br>
> static unsigned computeCost(Value *MemInst) {<br>
> if (StoreInst *SI = dyn_cast<StoreInst>(MemInst))<br>
> return StoreCost(SI, …).cost();<br>
> LoadInst *LI = cast<LoadInst>(MemInst);<br>
> return LoadCost(LI, …).cost();<br>
> }<br>
><br>
> private:<br>
><br>
> class StoreCost : public MemOpCost {<br>
> StoreCost(StoreInst *Inst) : MemOpCost(…) {<br>
> Alignment = Inst->getAlignment();<br>
> ...<br>
> }<br>
> };<br>
><br>
> class LoadCost : public MemOpCost {<br>
> StoreCost(LoadInst *Inst) : MemOpCost(…) {<br>
> Alignment = Inst->getAlignment();<br>
> ...<br>
> }<br>
> };<br>
> };<br>
><br>
> On Feb 4, 2013, at 11:16 AM, Nadav Rotem <<a href="mailto:nrotem@apple.com">nrotem@apple.com</a>> wrote:<br>
><br>
>> + if ((Store = dyn_cast<StoreInst>(MemInst)) != 0) {<br>
>><br>
>> if (Store = dyn_cast<StoreInst>(MemInst)) {<br>
>><br>
>><br>
>> + unsigned getOpcode() {<br>
>> + if (Store)<br>
>> + return Store->getOpcode();<br>
>> +<br>
>> + return Load->getOpcode();<br>
>> + }<br>
>> +<br>
>> + unsigned getAlignment() {<br>
>> + if (Store)<br>
>> + return Store->getAlignment();<br>
>> +<br>
>> + return Load->getAlignment();<br>
>> + }<br>
>> +<br>
>> + unsigned getPointerAddressSpace() {<br>
>> + if (Store)<br>
>> + return Store->getPointerAddressSpace();<br>
>> +<br>
>> + return Load->getPointerAddressSpace();<br>
>> + }<br>
>><br>
>> We can initialize all of these values in the constructor using a single IF statement. I think that it would be better than inheritance.<br>
>><br>
>><br>
>> Thanks,<br>
>> Nadav<br>
>><br>
>><br>
>><br>
>> On Feb 4, 2013, at 7:47 AM, Arnold Schwaighofer <<a href="mailto:aschwaighofer@apple.com">aschwaighofer@apple.com</a>> wrote:<br>
>><br>
>>> Hi Renato,<br>
>>><br>
>>> thanks for the feedback - really appreciate it!<br>
>>><br>
>>><br>
>>> On Feb 4, 2013, at 9:22 AM, Renato Golin <<a href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>> wrote:<br>
>>>> 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.<br>
>>><br>
>>> I was only going half the way ;). Good idea - will do.<br>
>>><br>
>>><br>
>>>> 2. Assert Liberally!<br>
>>>><br>
>>>> + // It better be a load now.<br>
>>>> + Load = cast<LoadInst>(MemInst);<br>
>>>><br>
>>>> to<br>
>>>><br>
>>>> + Load = dyn_cast<LoadInst>(MemInst);<br>
>>>> + assert(Load && "Invalid memory instruction type");<br>
>>>><br>
>>>> or use llvm_unreachable().<br>
>>><br>
>>> 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.<br>
>>><br>
>>><br>
>>>><br>
>>>> 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:<br>
>>>><br>
>>>> + unsigned getOpcode() {<br>
>>>> + if (Store)<br>
>>>> + return Store->getOpcode();<br>
>>>> +<br>
>>>> + return Load->getOpcode();<br>
>>>> + }<br>
>>>> +<br>
>>>> + unsigned getAlignment() {<br>
>>>> + if (Store)<br>
>>>> + return Store->getAlignment();<br>
>>>> +<br>
>>>> + return Load->getAlignment();<br>
>>>> + }<br>
>>>> +<br>
>>>> + unsigned getPointerAddressSpace() {<br>
>>>> + if (Store)<br>
>>>> + return Store->getPointerAddressSpace();<br>
>>>> +<br>
>>>> + return Load->getPointerAddressSpace();<br>
>>>> + }<br>
>>><br>
>>> 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.<br>
>>><br>
>>> -Arnold<br>
>>><br>
>><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br></blockquote></div><br></div>