[PATCH] D48085: [DAGCombiner] restrict (float)((int) f) --> ftrunc with no-signed-zeros

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D48085#1136910, @lebedev.ri wrote:

> In https://reviews.llvm.org/D48085#1136900, @spatel wrote:
>
> > In https://reviews.llvm.org/D48085#1136860, @lebedev.ri wrote:
> >
> > > Should this adjust the ReleaseNotes?
> >
> >
> > The docs provide extra warning/workaround, and this change doesn't affect that language IMO (there's less chance we're going to break code, but we don't have to disclose that).
> >
> > For reference, here are links to the docs that we added with the previous patches (last updated with https://reviews.llvm.org/D46236 I think):
> >  https://clang.llvm.org/docs/ReleaseNotes.html#new-compiler-flags
> >  https://clang.llvm.org/docs/UsersManual.html#controlling-code-generation
> >  http://llvm.org/docs/ReleaseNotes.html#non-comprehensive-list-of-changes-in-this-release
> >
> > Let me know if you see anything that can be improved.
>
>
> The text in first and last links suggests that such an optimization is always being done.
>  But now it will only be done in presence of `no-signed-zeros-fp-math` attribute.
>  Which is controlled by `-ffast-math` (or maybe some more fine-grained option, too?)


Right - it should still be allowed with "-fno-signed-zeros" at a minimum.

If we change the docs to provide the exact details of the potential optimization, then we have to adjust the docs anytime that changes (eg, if we implement the fabs trick for some subset of data types on some subset targets). So my preference is to not document the implementation at that level, but if you think that the text is misleading, I'll add a note.


https://reviews.llvm.org/D48085