[PATCH] D61331: [SelectionDAG] remove constant folding limitations based on FP exceptions

Tue Apr 30 15:13:41 PDT 2019

spatel added a comment.

In D61331#1485112 <https://reviews.llvm.org/D61331#1485112>, @andrew.w.kaylor wrote:

> In D61331#1485000 <https://reviews.llvm.org/D61331#1485000>, @efriedma wrote:
>
> > We generally try to avoid keeping dead code in-tree, but I guess I'm not strongly opposed to it here.
>
>
> I honestly don't have strong feelings about this particular change either. I'm more concerned about the way we think about strict FP handling.  We've still got a lot of work to do to finish strict FP handling, and I would hate to see any code that currently has logic to handle strict exception semantics lose it. On the other hand, looking closer at the current code, I see that the logic there today isn't actually correct for strict exception semantics. So I guess if we decide to go with my suggestion then every place that's currently checking "Status != APFloat::opInvalidOp" should really be checking "Status == APFloat::opOK".

That would definitely be an improvement, but I'm not sure if that's enough. For example, if we intend to track denormals here, then we'd have to check the returned constant value rather than the status value?

It seemed to me that the strict and regular cases would end up sharing just the 1 line of code that computes the constant, so it might make more sense to group strict nodes together rather than interleave those cases with the regular ops.

> BTW, in addition to what Craig pointed out above with regard to the chain, he visited me and mentioned additional complications related to the fact that we aren't keeping the rounding mode and exception semantics in the DAG. We really need a way to fix that. I'm inclined to believe constant folding will still end up going through this function, but it'll be more work to get here than I realized.

Yes, I think there's a lot of plumbing to do...

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

https://reviews.llvm.org/D61331