[PATCH] D87130: [SelectionDAGBuilder] Remember to copy the FMF flags in visitBinary().

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D87130#2256747 <https://reviews.llvm.org/D87130#2256747>, @uweigand wrote:

> In D87130#2256441 <https://reviews.llvm.org/D87130#2256441>, @jonpa wrote:
>
>> In D87130#2256173 <https://reviews.llvm.org/D87130#2256173>, @uweigand wrote:
>>
>>> I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854.  I guess there may be other parts of this commit that should be reverted?
>>
>> The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...
>
> Well, the whole approach of D46854 <https://reviews.llvm.org/D46854> was to **remove** setting of the FMF in DAG.getNode() everywhere, and **instead** set the flags via setFlags() after the fact.  The point was to be that setting the flags in one place reduces code duplication.   However, we're now having the discussion that using setFlags() after the fact may be incorrect in the presence of memoized nodes, and instead we should be always setting flags directly in getNode() -- this basically argues for exactly the opposite direction of what that commit did.

The after the fact handling also caused issue with constrained fp intrinsics which is why they were already excluded. It also had an issue with the vector reduction flag we used to have until I removed it in  https://reviews.llvm.org/D76649.


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

https://reviews.llvm.org/D87130