Page MenuHomePhabricator

propagate IR-level fast-math-flags to DAG nodes, disabled by default
ClosedPublic

Authored by spatel on Jun 11 2015, 3:02 PM.

Details

Summary

This is an updated version of the patch that was checked in at:
http://reviews.llvm.org/rL237046

but subsequently reverted because it exposed the bug in:
http://reviews.llvm.org/D9893

This time I've put an enablement flag ("EnableFMFInDAG") around the code in SelectionDAGBuilder where we copy the set of FP optimization flags from IR instructions to DAG nodes. So, in theory, there should be no functional change from this patch as-is, but it will allow testing with the added functionality to proceed via "-enable-fmf-dag" passed to llc. So for example the testcase in D9893 should be fine by default and crash with FMF enabled.

This patch is derived from part of D8900.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 27550.Jun 11 2015, 3:02 PM
spatel retitled this revision from to propagate IR-level fast-math-flags to DAG nodes, disabled by default.
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added a reviewer: nlewycky.
spatel added a subscriber: Unknown Object (MLST).
nlewycky edited edge metadata.Jun 15 2015, 3:11 PM

Are you sure you don't need to re-enable any tests with this commit? I'm concerned that there's no test change.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
410–412 ↗(On Diff #27550)

Are you saying that the FoldingSetNodeID is computed elsewhere too, without using this function? And that we don't want to call AddInteger one more time because we don't want to make it different from that other place of computation?

Are you sure you don't need to re-enable any tests with this commit? I'm concerned that there's no test change.

There should be no visible changes from adding the FP flags because we're not detecting them for optimizations anywhere in the DAG. That's why I naively marked the previous patches as 'NFC'. :)

The existing tests for Exact / NSW / NUW verify that we're still able to do optimizations based on those bits.

My first proposed patch for optimization using the FP flags ( http://reviews.llvm.org/D9708 ) just happened to be the same spot where you discovered a bug ( http://reviews.llvm.org/D9893 )!

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
410–412 ↗(On Diff #27550)

Yep, that's the idea. The new 'getNode' API has a nullptr default for the flags:
SDValue getNode(unsigned Opcode, SDLoc DL, EVT VT, SDValue N1, SDValue N2,

const SDNodeFlags *Flags = nullptr);

So we're not updating every use of that function to explicitly include a Flags ptr. If some path into here is using Flags but none are set, then we want to match the folding set node that would be created by those un-Flag'ified uses of getNode().

nlewycky accepted this revision.Jun 15 2015, 5:49 PM
nlewycky edited edge metadata.

This LGTM under the assumption that r237046 looked good too.

This revision is now accepted and ready to land.Jun 15 2015, 5:49 PM
This revision was automatically updated to reflect the committed changes.

This LGTM under the assumption that r237046 looked good too.

Thanks, Nick! r237046 was a limited form of:
http://reviews.llvm.org/rL236546
which was reviewed in D8900.

This patch checked in at r239828.

Please let me know if I can still take you up on this offer:
"Could you add FMF under a flag and then let me test it before turning it on? I can test for compiler crashes in 24 hours, or for miscompiles over a weekend."

Sanjay Patel wrote:

In http://reviews.llvm.org/D10403#188479, @nlewycky wrote:

This LGTM under the assumption that r237046 looked good too.

Thanks, Nick! r237046 was a limited form of:
http://reviews.llvm.org/rL236546
which was reviewed in http://reviews.llvm.org/D8900.

This patch checked in at r239828.

Please let me know if I can still take you up on this offer:
"Could you add FMF under a flag and then let me test it before turning it on? I can test for compiler crashes in 24 hours, or for miscompiles over a weekend."

Right! Yes, I can do that.

REPOSITORY

rL LLVM

http://reviews.llvm.org/D10403

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits