[llvm-branch-commits] [llvm] [FPEnv][SDAG] Implement FNEARBYINT with optional chain (PR #163081)

[Joshua Cranmer via llvm-branch-commits]

jcranmer-intel wrote:

> IMO, introducing "optional" chains makes this migration _more_ risky than adding required chain operands, not less risky. Modifying the fixed set of operands for a node is a pretty obvious change, though perhaps somewhat annoying.
> 
> With an optional chain argument, I think you will be able to get away with a partially-correct implementation easier, but making it _actually_ correct will be harder. Everywhere that deals with these nodes needs to be modified to understand that the first operand may be conditionally either a chain or not, and all the other operands may be consequently offset by 1.
> 
> I also don't see any way around validating that every target is handling this correctly, regardless of whether it's optional or not.

I generally try to avoid working in the backend as much as possible, so I don't have the strongest stake in this affair. But I fully concur with these points. The trade-off being made is trading off an approach that virtually guarantees hidden bugs if people forget about the presence of chains versus one that requires doing the full auditing work that's anyways needed.

https://github.com/llvm/llvm-project/pull/163081