[Driver, CodeGen] add options to enable/disable an FP cast optimization

[spatel]

Summary

As discussed in the post-commit thread for:
rL330437 ( http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180423/545906.html )

We need a way to opt-out of a float-to-int-to-float cast optimization because too much existing code relies on the platform-specific undefined result of those casts when the float-to-int overflows.

I speculatively committed the LLVM changes associated with adding this function attribute, but I'll change the name/implementation if there's a better alternative:
rL330947
rL330950
rL330951

Also as suggested, I changed the LLVM doc to mention the specific sanitizer flag that catches this problem:
rL330958

I tested the end-to-end results on x86 and see the expected outcome: 'roundss' is no longer produced in place of cvttss2si + cvtsi2ss with default optimization.

[lebedev.ri]

test/CodeGen/no-junk-ftrunc.c

2

For a good measure, i'd add one more RUN line to test that it is currently the default.
(Yes, i noticed that it is already tested in test/Driver/fast-math.c)

[spatel]

test/CodeGen/no-junk-ftrunc.c

2

The driver alone is handling the default setting, so it passes this flag to the front-end only when we're going to disable the transform. Ie, the driver eats "-fno-fp-cast-overflow-workaround" and sends nothing in that case to the front-end.

So there's not currently any case where the function attribute will be "fp-cast-overflow-workaround=false", but I left that as a possibility in case we decide to lift the limit at a finer granularity (scalar vs. vector etc).

I may be misunderstanding the question/suggestion - do we want the front-end to independently have a default setting?

[jgorbe]

Can't comment much on the patch itself (I'm still not very familiar with the codebase, I'll leave that to the other reviewers), but thanks a lot for responding so quickly! :)

[chandlerc]

docs/UsersManual.rst

1260–1265

I would phrase this the other way around (and I think the flag name is already phrased the other way around?):

"""
Enable a workaround for incorrect code that casts floating point values to integers where the floating point value is not representable in the integer type. This code is incorrect according to the language standard, but this flag will attempt to generate code to cause <insert expected behavior with the flag enabled>.
"""

Essentially, this should be more like '-fwrapv'. Also, I think the default should be what the specification says. People can explicitly pass this flag if their code is broken in this way.

[spatel]

docs/UsersManual.rst

1260–1265

Ah - did I misinterpret the earlier comments?

I thought we need to have the work-around 'on' by default as the immediate fix for broken programs?

[chandlerc]

docs/UsersManual.rst

1260–1265

Maybe others feel strongly about the default. I'm happy to explicitly pass a flag for our code until we get it fixed here.

I would suggest starting by adding the flag to toggle the behavior but not changing the default (which as of now is 'optimize, no workaround') and then we can invert the default if we get enough feedback that this is causing users problems.

Either way, I'd word this as suggested above.

[scanon]

I like Chandler's wording. Something like:

"... this flag will attempt to cause <clang to generate code as though the result of such conversions were defined to be an unspecified integer value.>"

[spatel]

Patch updated:

1. Improve the documentation language - more suggestions welcome!
2. Change the default setting so the work-around is 'off' (ie, by default assume source is compliant and optimize accordingly).
3. Remove the 'no' version of the flag. Given the change in the default, this seems more natural to me, and it simplifies the patch/tests...but I might have been too pessimistic before and this is too optimistic? Let me know...

[chandlerc]

In D46135#1080108, @spatel wrote:

3. Remove the 'no' version of the flag. Given the change in the default, this seems more natural to me, and it simplifies the patch/tests...but I might have been too pessimistic before and this is too optimistic? Let me know...

Please keep the no- version so that the flag can be toggled by appending to the command line.

test/CodeGen/no-junk-ftrunc.c

2

How about a test that checks the attribute is *absent* in the default mode then?

I think it's useful to have the both sides of the test, however a particular side is spelled.

[spatel]

Patch updated:

1. Restore the 'no' option to allow toggling.
2. Add a RUN to the codegen test to show that the function attribute is not appended by default.

[chandlerc]

LGTM, thanks so much!

[spatel]

Should I add a bullet for this new flag/attribute to the clang release notes, the LLVM release, both? Or do we view this as temporary and not making it to the release?

[lebedev.ri]

In D46135#1081165, @spatel wrote:

Should I add a bullet for this new flag/attribute to the clang release notes, the LLVM release, both? Or do we view this as temporary and not making it to the release?

(Not everyone is always using trunk snapshots)
I'd guess it would be present for at least one release, so I would expect to see that in both the clang and llvm release notes.

[spatel]

In D46135#1081193, @lebedev.ri wrote:

(Not everyone is always using trunk snapshots)
I'd guess it would be present for at least one release, so I would expect to see that in both the clang and llvm release notes.

Thanks - sounds right to me:
rL331056
rL331059