[PATCH] D26224: NewGVN

[Davide Italiano via llvm-commits]

On Tue, Dec 13, 2016 at 8:47 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> On Tue, Dec 13, 2016 at 10:31 AM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>>
>>>
>>>
>>> ================
>>> Comment at: lib/Transforms/Scalar/NewGVN.cpp:1455
>>> +
>>> +    if (DFSIn < other.DFSIn)
>>> +      return true;
>>> ----------------
>>> This is just implementing a lexicographical comparison, right?
>>
>> Yes
>>>
>>>
>>> If so, I would do:
>>>
>>> ```
>>> return std::tie(DFSIn, DFSOut, ...) < std::tie(other.DFSIn, other.DFSOut,
>>> ...);
>>
>>
>>
>> When i started this code, we didn't have tuple (it's that old!), but yeah,
>> this is what should be done now.
>>
>>>
>>> ```
>>>
>>>
>>> ================
>>> Comment at: lib/Transforms/Scalar/NewGVN.cpp:1464
>>> +        else if (LocalNum == other.LocalNum) {
>>> +          if (Val < other.Val)
>>> +            return true;
>>> ----------------
>>> This seems super sketchy, comparing < on pointers. What's up with that?
>>
>>  It's really just here for completeness,but agree it looks sketchy.
>>
>>>
>>> Won't that make the result nondeterministic?
>>
>> No, actually, though i admit it's not obvious to see this.
>>  It's not possible to push or pop a value for elimination differently  in
>> a way that matters based on the .val (and you should be able to prove this).
>> Each LLVM instruction only produces one value, and thus  the lowest-level
>> differentiator that really matters for the stack (and what we use as as a
>> replacement) is the local dfs number.
>> Everything else in the structure is instruction level, and only affects
>> the order in which we will replace operands of a given instruction.
>>
>> For a given instruction (IE things with equal dfsin, dfsout, localnum),
>> the order of replacement of uses does not matter (and no other pass
>> guarantees it, AFAIK)
>>
>> IE given,
>>
>> a = 5
>> b = a + a
>>
>> When you hit b, you will have two valuedfs with the same dfsin, out, and
>> localnum.
>> The .val will be the same as well.
>> The .u's will be different.
>>
>> You will replace both, and it does not matter what order you replace them
>> in (IE whether you replace operand 2, then operand 1, or operand 1, then
>> operand 2).
>>
>>
>> Similarly for the case of same dfsin, dfsout, localnum, but different
>> .val's
>>
>> a = 5
>> b  = 6
>> c = a + b
>>
>> in c, we will a valuedfs for a, and one for b,with everything the same but
>> .val  and .u.
>> It does not matter what order we replace these operands in.
>>
>> You will always end up with the same IR, and this is guaranteed.
>
>
> Thanks for the explanation. Davide, can we get this in a comment?
>

Sure, will add it. I'm going to address the last comments and leave
this another day or two in case somebody else has comments. Many
people reviewed this code and I think it's in a decent state to
check-in and iterate in tree (Hal is OK with this, Dan what do you
think?)
I'd like personally to thank everybody who commented in the last month or such!

--
Davide