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

[Duncan Sands]

Hi Michael,

On 01/10/12 22:14, Michael Ilseman wrote:
> I could also swap based on the addresses of the operands, which is how GVN does
> it.

this isn't how GVN does it.  For commutative instructions GVN sorts the value
numbers of the operands, not their addresses.

Ciao, Duncan.

  Swapping itself would still change execution to execution, but it will be
> consistent within a single execution.
>
> On Oct 1, 2012, at 1:00 PM, Chandler Carruth <chandlerc at google.com
> <mailto:chandlerc at google.com>> wrote:
>
>> On Mon, Oct 1, 2012 at 11:07 AM, Michael Ilseman <milseman at apple.com
>> <mailto:milseman at apple.com>> wrote:
>>
>>     When hashing the operands, first generate all the hashes and if it's
>>     commutative then put the smaller hash in first (and also swap the
>>     predicate for CmpInst).
>>
>>
>> This sounds very risky. Please don't rely on the particular values of hashes
>> to produce different output.
>>
>> The correct way to detect this in a hash table is to use a symmetric hash or
>> to use some existing program input to arbitrarily fix the ordering, and always
>> hash both and always in that order.
>>
>>     The one thing I'm unsure of with this approach is that SimpleValue seems
>>     meant to just be a wrapper around an Instruction pointer, and this is
>>     somewhat violating intuition.
>>
>>     On Oct 1, 2012, at 11:03 AM, Owen Anderson <resistor at mac.com
>>     <mailto:resistor at mac.com>> wrote:
>>
>>     > How would you decide which commutation is the canonical one, without
>>     doing full reassociation?  I suppose you could try to find a symmetric
>>     hash function…
>>     >
>>     > --Owen
>>     >
>>     > On Oct 1, 2012, at 11:00 AM, Michael Ilseman <milseman at apple.com
>>     <mailto:milseman at apple.com>> wrote:
>>     >
>>     >> Would also modifying
>>     DenseMapInfo<SimpleValue>::getHashValue(SimpleValue Val) resolve the
>>     hashing issue?
>>     >>
>>     >> On Oct 1, 2012, at 10:56 AM, Owen Anderson <resistor at mac.com
>>     <mailto:resistor at mac.com>> wrote:
>>     >>
>>     >>>
>>     >>> On Oct 1, 2012, at 10:42 AM, Chris Lattner <clattner at apple.com
>>     <mailto:clattner at apple.com>> wrote:
>>     >>>
>>     >>>>
>>     >>>> On Sep 30, 2012, at 7:07 PM, Michael Ilseman <milseman at apple.com
>>     <mailto:milseman at apple.com>> wrote:
>>     >>>>
>>     >>>>> Attached is a new version. I went with "CanonicalExpr" and added
>>     function level comments.
>>     >>>>
>>     >>>> Hi Michael,
>>     >>>>
>>     >>>> Why is this needed?  Why isn't it enough to change the predicate in
>>     "DenseMapInfo<SimpleValue>::isEqual" to handle commutative operations?
>>     >>>
>>     >>> I don't think that's sufficient.  isEqual is used to resolve
>>     collisions.  It won't help here, because the commuted instructions will
>>     hash to different values up front.  Note that after reassociation we could
>>     rely on  commuted (integer) instructions being canonicalized, but that
>>     doesn't help given EarlyCSE's place in the pipeline.
>>     >>>
>>     >>> --Owen
>>     >>
>>     >
>>
>>
>>     _______________________________________________
>>     llvm-commits mailing list
>>     llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>     http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>