[PATCH] fast-math patches!

Michael_Ilseman2 · November 15, 2012, 6:17pm

Attached are some patches for adding in an IR-level mechanism for representing fast-math flags, as discussed in my prior RFC. Patches include infrastructure, API support, textual and bitcode reader/writer support, example optimization, and test cases.

0002-Fast-math-flags-added-to-FPMathOperator.patch (4.82 KB)

0003-Fast-math-interfaces-for-Instructions.patch (7.69 KB)

0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch (5.18 KB)

0005-Fast-math-flags-for-the-bitcode.patch (3.14 KB)

0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch (5.28 KB)

0007-Fast-math-optimization-fold-multiply-by-zero.patch (4.3 KB)

0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch (1.45 KB)

Evan_Cheng1 · November 15, 2012, 6:38pm

Hi Michael,

The patch looks good in general. But I'm a bit concerned about the textural representation about these flags. 'N', 'I', 'S', 'R', 'A' seem cryptic to me. Does it make sense to expand them a bit 'nnan', 'inf', etc.? They definitely need to be documented.

Evan

Michael_Ilseman2 · November 15, 2012, 6:51pm

Hi Michael,

The patch looks good in general. But I'm a bit concerned about the textural representation about these flags. 'N', 'I', 'S', 'R', 'A' seem cryptic to me. Does it make sense to expand them a bit 'nnan', 'inf', etc.? They definitely need to be documented.

I think it does make sense to expand them to be more readable. Also, the textual representation doesn't have to precisely follow the internal names. What about:
nnan : no nans
ninf : no infs
nsz : no signed zeros
ar: allow reciprocal
fast : unsafe algebra (and implicitly all the others)

I'll get started on documentation.

Evan_Cheng1 · November 15, 2012, 6:51pm

Hi Michael,

The patch looks good in general. But I'm a bit concerned about the textural representation about these flags. 'N', 'I', 'S', 'R', 'A' seem cryptic to me. Does it make sense to expand them a bit 'nnan', 'inf', etc.? They definitely need to be documented.

I think it does make sense to expand them to be more readable. Also, the textual representation doesn't have to precisely follow the internal names. What about:
nnan : no nans
ninf : no infs
nsz : no signed zeros
ar: allow reciprocal
fast : unsafe algebra (and implicitly all the others)

These seem reasonable to me. Thanks!

Evan

Michael_Ilseman2 · November 15, 2012, 10:44pm

New patches with review feedback incorporated:
  * Changed single letter flags to short abbreviations ('S' ==> 'nsz')
  * Indentation fixes
  * Comments don't state function names

0002-Fast-math-flags-added-to-FPMathOperator.patch (4.82 KB)

0003-Fast-math-interfaces-for-Instructions.patch (7.25 KB)

0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch (5.81 KB)

0005-Fast-math-flags-for-the-bitcode.patch (3.13 KB)

0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch (5.51 KB)

0007-Fast-math-optimization-fold-multiply-by-zero.patch (4.3 KB)

0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch (1.46 KB)

Joe_Abbey1 · November 15, 2012, 10:50pm

Trying to apply patches..

What's your base revision?

Joe

Joe_Abbey1 · November 15, 2012, 11:23pm

Though semantically equivalent in this case, however I think you should use logical ors here not bitwise.

bool any() {
return UnsafeAlgebra | NoNaNs | NoInfs | NoSignedZeros |
AllowReciprocal;
}

Gripe: This pattern is probably super fast and has precedence… but the code is non-obvious:

SubclassOptionalData =
(SubclassOptionalData & ~BitToSet) | (B * BitToSet);

This is likely one iota slower… but it sure is easier to get the intent.

B ? SubclassOptionalData |= BitToSet :
SubclassOptionalData &= ~BitToSet;

Otherwise looks good to me.

Joe

Michael_Ilseman2 · November 15, 2012, 11:27pm

Sorry about that. Here's a new batch based off of r168110

0001-Fast-math-flags-added-to-FPMathOperator.patch (4.63 KB)

0002-Fast-math-interfaces-for-Instructions.patch (7.23 KB)

0003-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch (4.77 KB)

0004-Fast-math-flags-for-the-bitcode.patch (3.13 KB)

0005-Fast-math-test-case-for-bitcode-and-textual-reading-.patch (5.51 KB)

0006-Fast-math-optimization-fold-multiply-by-zero.patch (4.3 KB)

0007-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch (1.46 KB)

Michael_Ilseman2 · November 15, 2012, 11:31pm

Though semantically equivalent in this case, however I think you should use logical ors here not bitwise.

bool any() {

return UnsafeAlgebra | NoNaNs | NoInfs | NoSignedZeros |

AllowReciprocal;

}

Will do.

Gripe: This pattern is probably super fast and has precedence… but the code is non-obvious:

SubclassOptionalData =
(SubclassOptionalData & ~BitToSet) | (B * BitToSet);

This is an existing pattern that’s used elsewhere in the file, so I had assumed it was well understood and preferred.

Michael_Ilseman2 · November 16, 2012, 2:54am

Another round of improved patches, and a patch for documentation changes to LangRef.

Make comments more up to date
Use ‘arcp’ instead of ‘ar’
Use logical ||

Still based off of r168110

0001-Fast-math-flags-added-to-FPMathOperator.patch (4.55 KB)

0002-Fast-math-interfaces-for-Instructions.patch (7.23 KB)

0003-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch (5 KB)

0004-Fast-math-flags-for-the-bitcode.patch (3.12 KB)

0005-Fast-math-test-case-for-bitcode-and-textual-reading-.patch (5.51 KB)

0006-Fast-math-optimization-fold-multiply-by-zero.patch (4.26 KB)

0007-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch (1.46 KB)

0008-Fast-math-flags-documentation-added-to-LangRef.patch (8.47 KB)

Joe_Abbey1 · November 16, 2012, 4:07am

Michael,

Overall the code looks good.

80-cols:

2046 FMF.UnsafeAlgebra = 0 != (Record[OpNum] & (1 << bitc::FMF_UNSAFE_ALGEBRA));
2047 FMF.NoNaNs = 0 != (Record[OpNum] & (1 << bitc::FMF_NO_NANS));
2048 FMF.NoInfs = 0 != (Record[OpNum] & (1 << bitc::FMF_NO_INFS));
2049 FMF.NoSignedZeros = 0 != (Record[OpNum] & (1 << bitc::FMF_NO_SIGNED_ZEROS));
2050 FMF.AllowReciprocal = 0 != (Record[OpNum] & (1 << bitc::FMF_ALLOW_RECIPROCAL));

I found some more in BitcodeReader::ParseConstants(), which I’ll scrub after this commits.

Will there be a // fmul N 1, x ==> x? perhaps a // fadd N S 0, x ==> x? as well as other arithmetic identities?

Cheers,

Joe

Michael_Ilseman2 · November 16, 2012, 5:35pm

Michael,

Overall the code looks good.

80-cols:

2046 FMF.UnsafeAlgebra = 0 != (Record[OpNum] & (1 << bitc::FMF_UNSAFE_ALGEBRA));
2047 FMF.NoNaNs = 0 != (Record[OpNum] & (1 << bitc::FMF_NO_NANS));
2048 FMF.NoInfs = 0 != (Record[OpNum] & (1 << bitc::FMF_NO_INFS));
2049 FMF.NoSignedZeros = 0 != (Record[OpNum] & (1 << bitc::FMF_NO_SIGNED_ZEROS));
2050 FMF.AllowReciprocal = 0 != (Record[OpNum] & (1 << bitc::FMF_ALLOW_RECIPROCAL));

I found some more in BitcodeReader::ParseConstants(), which I’ll scrub after this commits.

Will there be a // fmul N 1, x ==> x? perhaps a // fadd N S 0, x ==> x? as well as other arithmetic identities?

Yes, that is the intent. You can see more examples in the original RFC. For you specific examples:

fmul N 1, x ==> x

I think that the ‘N’ (now spelled ‘nnan’) flag isn’t relevant to the optimization, as NaN * 1 ==> NaN.

fadd N S 0, x ==> x

Again, ‘N’ shouldn’t apply. I don’t think ‘S’ (‘nsz’) is needed here either, but it would be if it was x - 0 ==> x.