Page MenuHomePhabricator

[ConstantFolding] Fold constrained arithmetic intrinsics
ClosedPublic

Authored by sepavloff on May 18 2021, 1:39 AM.

Details

Summary

Constfold constrained variants of operations fadd, fsub, fmul, fdiv,
frem, fma and fmuladd.

The change also sets up some means to support for removal of unused
constrained intrinsics. They are declared as accessing memory to model
interaction with floating point environment, so they were not removed,
as they have side effect. Now constrained intrinsics that have
"fpexcept.ignore" as exception behavior are removed if they have no uses.
As for intrinsics that have exception behavior other than "fpexcept.ignore",
they can be removed if it is known that they do not raise floating point
exceptions. It happens when doing constant folding, attributes of such
intrinsic are changed so that the intrinsic is not claimed as accessing
memory.

Diff Detail

Event Timeline

sepavloff created this revision.May 18 2021, 1:39 AM
sepavloff requested review of this revision.May 18 2021, 1:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 1:39 AM
Herald added a subscriber: wdng. · View Herald Transcript
kpn added inline comments.May 26 2021, 8:07 AM
llvm/lib/Transforms/Utils/Local.cpp
502

What about "maytrap"? The language reference explicitly calls out constant folding as allowed under "maytrap" since it doesn't introduce any new exceptions.

llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
240 ↗(On Diff #347343)

I hate to lose the testing we're currently doing with this file. Can you split out from this file the tests that these changes hit, and then change this existing test to take arguments instead of constants? That way we preserve testing the SDAG but also test that your constant folding works correctly.

nemanjai added inline comments.May 26 2021, 9:19 AM
llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll
240 ↗(On Diff #347343)

I agree. I'm not sure why this test case was written to only test this with constants, but it now becomes clear that it is an unfortunate choice.

Changes to the test llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll are presented in D103259.

sepavloff updated this revision to Diff 348918.Jun 1 2021, 2:48 AM

Allow folding for 'maytrap'. Rebased

sepavloff added inline comments.Jun 1 2021, 3:35 AM
llvm/lib/Transforms/Utils/Local.cpp
502

I think you are right. According to the documentation, constant folding is possible in this case.

foad added inline comments.Jun 4 2021, 2:02 AM
llvm/lib/Analysis/ConstantFolding.cpp
2458

You don't need the "if", just do it unconditionally?

spatel added a subscriber: spatel.Jun 4 2021, 8:54 AM
spatel added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
2417

From the tests, I'm assuming that this diff is here to support bfloat, but this change affects more than constrained intrinsics and more than only bfloat.

Either we need to limit this enhancement, or we need to split this into its own patch and add more tests to make sure it works as expected. I put in some basic coverage for copysign here:
8a4d05ddb3ff

I think things work as expected for simple cases like that one, but I don't know what happens if we are using the host mathlib to evaluate more complex functions (for example "pow" is in the switch under here).

sepavloff updated this revision to Diff 350233.Jun 7 2021, 3:51 AM

Addressed reviewers' comments, rebased

sepavloff added inline comments.Jun 7 2021, 4:05 AM
llvm/lib/Analysis/ConstantFolding.cpp
2417

I put checks

if (!Ty->isHalfTy() && !Ty->isFloatTy() && !Ty->isDoubleTy())
        return nullptr;

below. Constrained intrinsics have tests for bfloat type in this patch. As for other functions, test for them will be added later, and these checks will be removed.

2458

Yes, you are right. Updated.

spatel added inline comments.Jun 8 2021, 5:51 AM
llvm/lib/Analysis/ConstantFolding.cpp
2417

Thanks. I'm worried that someone could come back after this patch and add yet another if or switch and miss the differences in type handling.

How about adding a helper function to deal with ConstrainedFP and calling it from the very first clause in this function?

That also raises a question: what is the behavior of these intrinsics with undef and/or poison operands?

sepavloff updated this revision to Diff 350801.Jun 9 2021, 12:51 AM

Move common code into separate function

sepavloff added inline comments.Jun 9 2021, 12:56 AM
llvm/lib/Analysis/ConstantFolding.cpp
2417

How about adding a helper function to deal with ConstrainedFP and calling it from the very first clause in this function?

The shared code is moved to getEvaluationRoundingMode.

That also raises a question: what is the behavior of these intrinsics with undef and/or poison operands?

Constrained intrinsics should not differ from their counterparts in handling undef/poison. @kpn is working on D103169, which eventually should implement the relevant folding.

sepavloff updated this revision to Diff 350957.Jun 9 2021, 11:01 AM

Swap sequence of patches

Is there anything I can do for this patch?

spatel added a subscriber: scanon.

Ping @scanon to comment on whether the exception vs. fold-ability logic seems correct.

llvm/lib/Analysis/ConstantFolding.cpp
1905–1906

I think this would be easier to read if we made it more like the code above here:

Optional<RoundingMode> ORM = CI->getRoundingMode();
// If no rounding mode is specified by the intrinsic or the mode is dynamic,
// try to evaluate using the default mode. If it does not raise an inexact
// exception, rounding was not applied so the result is independent of
// rounding mode. 
if (!ORM || *ORM == RoundingMode::Dynamic) 
  return RoundingMode::NearestTiesToEven;
// Use the mode specified by the intrinsic.
return *ORM;

That's assuming I'm reading it correctly right now - are there tests with no RM specified on the intrinsic?

sepavloff updated this revision to Diff 354463.Jun 25 2021, 3:47 AM

Addressed reviewer's note. Rebased

sepavloff marked an inline comment as done.Jun 25 2021, 3:52 AM
sepavloff added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
1905–1906

That's assuming I'm reading it correctly right now - are there tests with no RM specified on the intrinsic?

No, all affected intrinsics have such argument. Bu in other places such check is made, so I put it here also.

spatel added a comment.Mon, Jul 5, 8:14 AM

Can you fix the formatting, so we don't have clang-tidy warnings? Those pop-up boxes are getting in the way of reading the code here in Phab.

llvm/lib/Analysis/ConstantFolding.cpp
1905–1906

I am not understanding something then.
If the metadata args for rounding mode and exception behavior are required by the LangRef:
https://llvm.org/docs/LangRef.html#llvm-experimental-constrained-fadd-intrinsic
...then why is the result of getRoundingMode() or getExceptionBehavior() an Optional value? If it is not valid IR without those args, we shouldn't be adding compile-time checks for things that can't happen.

sepavloff updated this revision to Diff 357945.Mon, Jul 12, 7:50 AM
sepavloff marked an inline comment as done.

Updated patch

  • Rebased,
  • Get rid of some clang-tidy warnings,
  • Updated test fdiv-strict.ll
sepavloff added inline comments.Mon, Jul 12, 9:18 AM
llvm/lib/Analysis/ConstantFolding.cpp
1905–1906

I am not understanding something then.
If the metadata args for rounding mode and exception behavior are required by the LangRef:
https://llvm.org/docs/LangRef.html#llvm-experimental-constrained-fadd-intrinsic
...then why is the result of getRoundingMode() or getExceptionBehavior() an Optional value? If it is not valid IR without those args, we shouldn't be adding compile-time checks for things that can't happen.

This function processes any ConstrainedFPIntrinsic. Some of them do not have rounding mode arguments, like constrained variants of floor, trunc. frem also do not depend on rounding mode, although it has rounding mode argument, probably some day it would be removed.

Using Optional as return value of getRoundingMode() or getExceptionBehavior() is now a part of ConstrainedFPIntrinsic interface. It is safer to process results of these function in more general way.

spatel added inline comments.Tue, Jul 13, 6:40 AM
llvm/lib/Analysis/ConstantFolding.cpp
1905–1906

Ok - thanks for explaining. I didn't think of those other intrinsics. Presumably, we'll add constant folding for those later, so this code will be shared.

1907

This code comment does not look accurate.
When we speculatively evaluate the expression, we check that *any* exception was not raised, not just the inexact exception, right?

This raises a question: if we evaluate the expression using NearestTiesToEven, then are we guaranteed that all potential exceptions (even underflow) are identical to any other rounding mode?

sepavloff updated this revision to Diff 358919.Thu, Jul 15, 4:23 AM

Rebased and reword comment

sepavloff added inline comments.Thu, Jul 15, 4:27 AM
llvm/lib/Analysis/ConstantFolding.cpp
1907

This code comment does not look accurate.
When we speculatively evaluate the expression, we check that *any* exception was not raised, not just the inexact exception, right?

Yes. If exceptions are tracked and constant evaluation can raise any of them, constant expression is not folded. The relevant logic is implemented by mayFoldConstrained, which is defined above. It is necessary to set hardware state, which is expected as side effect of the evaluation.

The purpose of this rounding mode substitution is to enable constant folding of expressions like 1.0 + 1.0 even when rounding mode is dynamic, so unknown at compile time.

I tried to reword the comment to make it clearer.

This raises a question: if we evaluate the expression using NearestTiesToEven, then are we guaranteed that all potential exceptions (even underflow) are identical to any other rounding mode?

IEEE-754 compliant system evaluates an operation in two steps. First it calculates intermediate result as if both the exponent range and the precision were unbounded. Then this result is rounded if it cannot be represented in the chosen floating point format. The first step does not depend on rounding mode but the second does.

Of the five FP exception division-by-zero and invalid obviously do not depend on rounding mode. Overflow is raised when the intermediate result is too large by magnitude to be represented in the chosen floating point format. Similar considerations apply to underflow as well. As for inexact exception, if it is raised, it means the intermediate result cannot be exactly represented and the rounding step changed it to either of nearby values. The direction is determined by rounding mode but rounding is required in any rounding mode.

spatel accepted this revision.Thu, Jul 15, 9:52 AM

LGTM - see inline for some test file suggestions.
Might want to get a 2nd opinion though - I haven't used constrained ops or FP exceptions in C outside of toy examples.

llvm/test/Transforms/InstSimplify/constfold-constrained.ll
2

If there's not too much noise, I recommend using utils/update_test_checks.py to auto-generate the FileCheck lines in this file - it should eliminate typos and make it easier to update the file with new tests.

So (1) add new tests, (2) auto-generate the baseline CHECKs for all tests, (3) apply this patch and re-run the script, so we just have the test diffs.

llvm/test/Transforms/InstSimplify/fdiv-strictfp.ll
27

Fix this test to not be misleading as a preliminary/NFC commit?

This revision is now accepted and ready to land.Thu, Jul 15, 9:52 AM

Thanks!

llvm/test/Transforms/InstSimplify/constfold-constrained.ll
2

(1) and (2) are implemented in https://reviews.llvm.org/rGa0b4f424f564

llvm/test/Transforms/InstSimplify/fdiv-strictfp.ll
27
qiucf added a subscriber: qiucf.Mon, Jul 19, 2:48 AM

I just got over to @kpn 's question on llvm-dev:
https://lists.llvm.org/pipermail/llvm-dev/2021-July/151727.html

And this patch should have tests with SNaN and QNaN inputs, so we have coverage for those cases (let me know if they are/were here, but I missed it).

define float @fadd.except.strict_qnan_qnan(float %x) {
  %r = call float @llvm.experimental.constrained.fadd.f32(float 0x7ff8000000000000, float 0x7ff8000000000000, metadata !"round.dynamic", metadata !"fpexcept.strict")
  ret float %r

}

define float @fadd.except.strict_snan_qnan(float %x) {
  %r = call float @llvm.experimental.constrained.fadd.f32(float 0x7ff4000000000000, float 0x7ff8000000000000, metadata !"round.dynamic", metadata !"fpexcept.strict")
  ret float %r

}

This revision was landed with ongoing or failed builds.Fri, Jul 23, 12:40 AM
This revision was automatically updated to reflect the committed changes.

I just got over to @kpn 's question on llvm-dev:
https://lists.llvm.org/pipermail/llvm-dev/2021-July/151727.html

And this patch should have tests with SNaN and QNaN inputs, so we have coverage for those cases (let me know if they are/were here, but I missed it).

This patch intentionally avoided checking NaN inputs because at the same time @kpn implemented https://reviews.llvm.org/D103169. That patch must have required tests.