[llvm-commits] [PATCH] Make EarlyCSE understand commutativity

[Michael Ilseman]

Yes, good catch.

On Sep 30, 2012, at 10:30 PM, Eric Christopher <echristo at gmail.com> wrote:

> Good name choice.
> 
> +  // end namespace gla_llvm
> 
> Probably didn't mean the gla?
> 
> -eric
> 
> On Sun, Sep 30, 2012 at 7:07 PM, Michael Ilseman <milseman at apple.com> wrote:
>> Attached is a new version. I went with "CanonicalExpr" and added function level comments.
>> 
>> 
>> On Sep 28, 2012, at 5:18 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>> 
>>> 
>>> On Sep 28, 2012, at 1:19 PM, Michael Ilseman <milseman at apple.com> wrote:
>>> 
>>>> Sure. What do you think a more fitting name would be for this? Something carrying the point of "CanonicalizedInstructionEncapsulation", but without being so unwieldy.
>>> 
>>> I don't have a good suggestion. InstrCanonicalizedExpr?
>>> 
>>> Evan
>>> 
>>>> 
>>>> 
>>>> On Sep 28, 2012, at 12:44 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>>> 
>>>>> Hi Michael,
>>>>> 
>>>>> In general this looks fine to me. However, the name "Expression" is way too generic. It's ok when it's part of GVN implementation details. It's really not a suitable name when it's factored out. Also, please add the missing function level comments.
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> Evan
>>>>> 
>>>>> On Sep 27, 2012, at 8:03 AM, Michael Ilseman <milseman at apple.com> wrote:
>>>>> 
>>>>>> I don't think that teaching EarlyCSE to do full on value numbering the way GVN does is needed or desired. I was able to factor out GVN's Expression class and its shared functionality, though, and EarlyCSE can use it instead of its SimpleValue as a canonicalized representation of an Instruction.
>>>>>> 
>>>>>> Attached is the new Expression.h (to reside in llvm/Transforms/Utils/), which contains the shared functionality. Also attached are GVN.patch and EarlyCSE.patch, which modify GVN and EarlyCSE to use it.
>>>>>> <Expression.h><GVN.patch><EarlyCSE.patch>
>>>>>> On Sep 24, 2012, at 1:51 PM, Owen Anderson <resistor at mac.com> wrote:
>>>>>> 
>>>>>>> 
>>>>>>> On Sep 24, 2012, at 12:55 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>>>>>> 
>>>>>>>> 
>>>>>>>> On Sep 24, 2012, at 9:46 AM, Michael Ilseman <milseman at apple.com> wrote:
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On Sep 22, 2012, at 3:01 AM, Duncan Sands <baldrick at free.fr> wrote:
>>>>>>>>>> urgh, I really don't like creating an instruction just to look it up, then
>>>>>>>>>> deleting it again.  Can you make the lookup logic more flexible instead?
>>>>>>>>>> By the way, how GVN takes care of commutativity is: when computing the value
>>>>>>>>>> number for an instruction, it sorts the operands of commutative instructions
>>>>>>>>>> (its sorts the operands according to their value number) before calculating
>>>>>>>>>> a value number from them, ensuring that if two commutative instructions are
>>>>>>>>>> the same except for a different operand order then they get the same value
>>>>>>>>>> number.  I don't know how EarlyCSE works, but maybe something along these lines
>>>>>>>>>> would also be possible?
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Thanks for the feedback!
>>>>>>>>> 
>>>>>>>>> I also don't like making a new instruction, but considered it the lesser of evils. Essentially, EarlyCSE uses a DenseMap over the instructions, but overloading hash to hash the instruction itself rather than its address. It also overloads isEqual to call Instruction::isIdenticalTo.
>>>>>>>>> 
>>>>>>>>> As far as changing the lookup mechanism, that would involve both sorting operand hashes on getHashValue, and extending Instruction::isIdenticalToWhenDefined to also calculate equality modulo commutativity (or, essentially inlining its body into the overloaded isEqual function rather than making the call). I don't know what is more desirable, but the original patch to EarlyCSE at least kept it very localized and would only trigger when a commutative op was encountered.
>>>>>>>>> 
>>>>>>>>> Another option is to restructure EarlyCSE to do expression numbering similarly to GVN, as you mentioned. Again, I don't know if this is more desirable over a localized call to clone(), as this would require a re-encapsulation of Instruction's data that would affect all of EarlyCSE.
>>>>>>>> 
>>>>>>>> I would strong advise against the new instruction approach. That's considered a definite no-no in LLVM. Do you have a good sense how much effort it is to implement expression numbering?
>>>>>>> 
>>>>>>> It's not so hard.  GVN already does it, and the code is relatively independent of the details of GVN.  You could probably extract it to a set of utility files and share it between both passes.
>>>>>>> 
>>>>>>> --Owen
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>