Page MenuHomePhabricator

[IR] Allow fast math flags on calls with floating point array type.
ClosedPublic

Authored by foad on Oct 18 2019, 3:19 AM.

Details

Summary

This extends the rules for when a call instruction is deemed to be an
FPMathOperator, which is based on the type of the call (i.e. the return
type of the function being called). Previously we only allowed
floating-point and vector-of-floating-point types. Now we also allow
arrays (nested to any depth) of floating-point and
vector-of-floating-point types.

This was motivated by llpc, the pipeline compiler for AMD GPUs
(https://github.com/GPUOpen-Drivers/llpc). llpc has many math library
functions that operate on vectors, typically represented as <4 x float>,
and some that operate on matrices, typically represented as
[4 x <4 x float>], and it's useful to be able to decorate calls to all
of them with fast math flags.

Event Timeline

foad created this revision.Oct 18 2019, 3:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 3:19 AM
Herald added a subscriber: wdng. · View Herald Transcript

The LangRef needs edits for this change.
I think this patch is also allowing the following constructs, but not testing for them.

define [3 x float] @arrayselect(i1 %cond, [3 x float] %x, [3 x float] %y) {
  %sel = select ninf i1 %cond, [3 x float] %x, [3 x float] %y
  ret [3 x float] %sel
}

define [1 x float] @arrayphi(i1 %cond, [1 x float] %x, [1 x float] %y) {
entry:
  br i1 %cond, label %t, label %f

t:
  br label %exit

f:
  br label %exit

exit:
  %phi = phi nnan [1 x float] [ %x, %t ], [ %y, %f ]
  ret [1 x float] %phi
}
foad updated this revision to Diff 225627.Oct 18 2019, 7:59 AM

Update langref.

foad added a comment.EditedOct 18 2019, 8:08 AM

I think this patch is also allowing the following constructs, but not testing for them.

Are there any existing tests for fast math flags on select or phi, that I could extend? I didn't even know they were allowed because they weren't mentioned in this part of the docs (https://llvm.org/docs/LangRef.html#fast-math-flags) and they aren't explicitly listed in the implementation of FPMathOperator::classof.

Incidentally, surely FPMathOperator::classof would be better implemented as a whitelist rather than a blacklist? At the moment I'm sure it is unintentionally allowing fast math flags on some instructions like ExtractValue. (This is now D69176.)

foad updated this revision to Diff 225647.Oct 18 2019, 9:08 AM

Rebase.

I think this patch is also allowing the following constructs, but not testing for them.

Are there any existing tests for fast math flags on select or phi, that I could extend? I didn't even know they were allowed because they weren't mentioned in this part of the docs (https://llvm.org/docs/LangRef.html#fast-math-flags) and they aren't explicitly listed in the implementation of FPMathOperator::classof.

Yes, select/phi are relatively recent additions, and we missed that LangRef update. Test diff ideas may be in these reviews:
D61917
D67564
D68748

foad updated this revision to Diff 225652.Oct 18 2019, 9:28 AM

Rebase after D69176.

Also should have some real IR tests that show this parses and preserves the flags

foad updated this revision to Diff 225859.Oct 21 2019, 5:26 AM

Update bitcode reader and ll parser and add tests.

foad added a comment.Oct 28 2019, 7:20 AM

Ping. I think I've added all the tests that folks asked for -- and fixed bugs they exposed in LLParser and BitcodeReader.

spatel added inline comments.Oct 29 2019, 9:14 AM
llvm/test/Bitcode/compatibility.ll
942

Duplicate this test with the array equivalents, so we are sure that path is round-tripping as expected?

foad updated this revision to Diff 227055.Oct 30 2019, 2:55 AM

Add a bitcode compatibility test for fast math flags on calls returning
array types.

foad marked an inline comment as done.Oct 30 2019, 2:57 AM
spatel accepted this revision.Oct 30 2019, 6:37 AM

LGTM

This revision is now accepted and ready to land.Oct 30 2019, 6:37 AM
This revision was automatically updated to reflect the committed changes.