[PATCH] D76319: [DAGCombiner] Fix non-determinism problem related to argument evaluation order in visitFDIV

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D76319#1941994 <https://reviews.llvm.org/D76319#1941994>, @dblaikie wrote:

> > For some reason the order in which we call getNegatedExpression for the involved operands, after a call to isCheaperToUseNegatedFPOps, seem to matter.
>
> Committing a change without understanding why the order of execution of getNegatedExpression matters seems a bit problematic - could you look into/explain why the order of two calls to that function that sounds pretty benign/order independent?


Forgot to update the description here, but in the final commit msg (see https://reviews.llvm.org/rGd168b7778035af6cc795b2367ca7f379ce1a629e) there is a reference to https://reviews.llvm.org/D76439 which follows up on some of the problems with getNegatedExpression() and getNegatibleCost() depending on each other. That seems to have been a problem for some time. I don't know the details myself, but I guess one problem is that those methods call each other. And since at least getNegatedExpression() may rewrite the expression tree, and for example the result from getNegatibleCost may vary from time to time if doing a call to getNegatedExpression() in between (so the checks done by isCheaperToUseNegatedFPOps is not neccessarily valid after calling getNegatedExpression the first time).
As far as I understand one could see this as two problems:

1. We want to get deterministic results from the compiler, and since getNegatedExpression() may rewrite the expression tree (well, at least add nodes and replace some uses) one might get different results depending on the order of calls to it (I think that was due to taking decisions based on "number of uses"). So my patch simply made sure the order is the same (not depending on argument evaluation order).
2. Getting rid of weird dependencies between getNegatedExpression and getNegatedExpression (and in some sense also isCheaperToUseNegatedFPOps) to make this part of the code less error-prone.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76319/new/

https://reviews.llvm.org/D76319