[PATCH] D26224: NewGVN

[Sean Silva via llvm-commits]

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?

-- Sean Silva


>
>
>
>>
>> https://reviews.llvm.org/D26224
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161213/15138777/attachment.html>