This is an archive of the discontinued LLVM Phabricator instance.

[ConstantFolding] Add folding for various math '__<func>_finite' routines generated from -ffast-math
ClosedPublic

Authored by chrischr on Apr 6 2017, 2:53 PM.

Details

Summary

This is the second of a set of patches to followup on the thread in http://lists.llvm.org/pipermail/llvm-dev/2017-March/111323.html. (The first patch is @ https://reviews.llvm.org/D31787)

This extends the constant folding of the math functions to also handle the variants that have been redirected to use the '__<func>_finite' variation that gets called under the -ffast-math flag.

Diff Detail

Repository
rL LLVM

Event Timeline

chrischr created this revision.Apr 6 2017, 2:53 PM
andrew.w.kaylor added inline comments.
lib/Analysis/ConstantFolding.cpp
1446 ↗(On Diff #94443)

The Name[0] check here is redundant, but I suppose it's mostly harmless.

test/Transforms/ConstProp/calls-math-finite.ll
35 ↗(On Diff #94443)

Won't this test fail on targets where these functions were marked as unavailable?

chrischr added inline comments.May 9 2017, 4:37 PM
lib/Analysis/ConstantFolding.cpp
1446 ↗(On Diff #94443)

Good catch. Will fix.

test/Transforms/ConstProp/calls-math-finite.ll
35 ↗(On Diff #94443)

Currently the test case is written without specifying a target triple, so the default OS value (UnknownOS) is in effect during the initialization of the TLI preventing the TLI.setUnavailable calls from executing.

andrew.w.kaylor added inline comments.May 9 2017, 4:40 PM
test/Transforms/ConstProp/calls-math-finite.ll
35 ↗(On Diff #94443)

I thought these tests would pick up the triple from the host if nothing was specified. Have you run this test on Windows?

chrischr added inline comments.May 9 2017, 5:00 PM
test/Transforms/ConstProp/calls-math-finite.ll
35 ↗(On Diff #94443)

I single stepped 'opt.exe' within Visual Studio, and saw it set to UnknownOS when looking at the local vars.

Also, test passes for me when running llvm-lit.py on Wndows.

andrew.w.kaylor added inline comments.May 9 2017, 5:03 PM
test/Transforms/ConstProp/calls-math-finite.ll
35 ↗(On Diff #94443)

OK. Thanks.

chrischr updated this revision to Diff 98470.May 10 2017, 8:48 AM

Removed redundant check on Name[0] as being '_', as suggested.

This revision is now accepted and ready to land.May 10 2017, 2:58 PM

A couple of things came up as I was preparing this for commit. They're pretty straightforward issues, so I'll just fix them in my sandbox before committing.

lib/Analysis/ConstantFolding.cpp
1446 ↗(On Diff #98470)

There's a '{' missing at the end of this 'if' line.

1467 ↗(On Diff #98470)

This needs an 'else return false'.

lib/Analysis/ConstantFolding.cpp
1467 ↗(On Diff #98470)

After a little more thought, I think I prefer this to 'else return false':

// The '12' here is the length of the shortest name that can match.
// We need to check the size before looking at Name[1] and Name[2]
// so we may as well check a limit that will eliminate mismatches.
if (Name.size() < 12 || Name[1] != '_')
  return false;
switch (Name[2]) {
<...>

Does that look OK?

chrischr added inline comments.May 11 2017, 2:48 PM
lib/Analysis/ConstantFolding.cpp
1467 ↗(On Diff #98470)

ah, yes, there does need to be a 'return false' somewhere. The change to the check for string length looks good to me.

This revision was automatically updated to reflect the committed changes.