Page MenuHomePhabricator

[TM] Restore default TargetOptions in TargetMachine::resetTargetOptions.
ClosedPublic

Authored by jlebar on Jan 9 2017, 7:30 PM.

Details

Summary

Previously if you had

  • a function with the fast-math-enabled attr, followed by
  • a function without the fast-math attr,

the second function would inherit the first function's fast-math-ness.

This means that mixing fast-math and non-fast-math functions in a module
was completely broken unless you explicitly annotated every
non-fast-math function with "unsafe-fp-math"="false". This appears to
have been broken since at r176986 (March 2013), when the
resetTargetOptions function was introduced.

This patch tests the correct behavior as best we can. I don't think I
can test FPDenormalMode and NoTrappingFPMath, because they aren't used
in any backends during function lowering. Surprisingly, I also can't
find any uses at all of LessPreciseFPMAD affecting generated code.

The NVPTX/fast-math.ll test changes are an expected result of fixing
this bug. When FMA is disabled, we emit add as "add.rn.f32", which
prevents fma combining. Before this patch, fast-math was enabled in all
functions following the one which explicitly enabled it on itself, so we
were emitting plain "add.f32" where we should have generated
"add.rn.f32".

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 83768.Jan 9 2017, 7:30 PM
jlebar retitled this revision from to [TM] Restore default TargetOptions in TargetMachine::resetTargetOptions..
jlebar updated this object.
jlebar added a reviewer: mkuper.
jlebar added subscribers: llvm-commits, majnemer, hfinkel.
jlebar updated this object.Jan 9 2017, 7:34 PM

Shout out to rr [1]. I went from "huh, this seems wrong" to finding the responsible code in about 30 seconds. Put a breakpoint where TargetOptions.UnsafeFPMath is read, hit the breakpoint, watch -l TargetOptions.UnsafeFPMath, reverse continue. Bam.

[1] http://rr-project.org/

majnemer accepted this revision.Jan 9 2017, 8:25 PM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Jan 9 2017, 8:25 PM
This revision now requires review to proceed.Jan 10 2017, 12:03 PM
echristo accepted this revision.Jan 10 2017, 12:06 PM
echristo edited edge metadata.

LGTM, thanks. I was looking for a reproducer for this for a while :)

This revision is now accepted and ready to land.Jan 10 2017, 12:06 PM

Thank you for the reviews, everyone!

This comment was removed by jlebar.
This revision was automatically updated to reflect the committed changes.