The change implements constant folding of ‘llvm.experimental.constrained.fcmp’
and ‘llvm.experimental.constrained.fcmps’ intrinsics.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1854 | This still worries me. Are _analysis_ passes allowed to change the input IR? What if the caller decides to not do a fold after calling this function to see if a fold is possible? And why is the ebIgnore case different from the other two? Constant folding won't add this ReadNone attribute when the constant folding code doesn't have an Instruction to alter. I don't know what to do about that. | |
llvm/lib/IR/Instructions.cpp | ||
4134 | Besides moving this code to a new function, are there any changes to it? It's hard to tell. | |
llvm/test/Transforms/InstSimplify/constfold-constrained.ll | ||
418 | There are no "maytrap" tests here. For the sake of future readers it would probably be useful to show that "maytrap" was taken into account and is working correctly. |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1854 | Analysis passes are indeed not allowed to change the IR. And this isn't a harmless change either if it gets left behind -- e.g. it invalidates MemorySSA. |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1854 |
Constant folding is not an analysis, it changes IR. I don't know why this file is in Analysis directory.
Nothing bad would happen. Only side effect would be removed, but we know for sure that it is absent, we just evaluated the operation. The function call would not be removed if its result is used.
If exception behavior is ebIgnore, such calls will get attribute SDNodeFlags::NoFPExcept in DAG and such instructions do not have side effects. But setting ReadNone for instructions with ebIgnore allows removal of these instructions earlier, at IR level, which could have positive effect.
Comparison of two floating point numbers do not use memory access. But it can change bits in the floating point state register (only Invalid bit can be set). This change is emulated as memory access so that instruction be ordered correctly. This is why constrained intrinsics declared with attribute IntrInaccessibleMemOnly. As no actual memory access occurs, it is harmless to set ReadNone in this case. | |
llvm/lib/IR/Instructions.cpp | ||
4134 | No, this is only a code moving. | |
llvm/test/Transforms/InstSimplify/constfold-constrained.ll | ||
418 | Added tests with "maytrap". |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1854 |
The constant folding analysis does not change IR. Users of the constant folding analysis change IR based on the analysis result.
My point here was that adding a readnone attribute invalidates MemorySSA, because it means that the instruction should no longer have a MemoryAccess -- it would result in a verification failure. Just calling ConstFold/InstSimplify should never have this kind of effect. Unless @spatel or @lebedev.ri tell me I'm wrong here, I believe this should be considered a blocker for further work in this area. |
Unless @spatel or @lebedev.ri tell me I'm wrong here, I believe this should be considered a blocker for further work in this area.
Should the change that effects fadd, fma, etc. tests be moved to a different patch that is a pre-requisite of the compare change?
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
2367 | Does ConstrainedFPCmpIntrinsic have any method for determining fcmp vs fcmps? If not should it? | |
llvm/lib/IR/Instructions.cpp | ||
4133 | This feels a little like it shouldn't be part of FCmpInst, but I don't have a concrete suggestion of where to put it instead. ConstantFold.cpp feels like the right home, but unfortunatley the header for that isn't visible to ConstantFolding.cpp. |
Yes, it is a good idea to move this debatable change into a separate patch, it is here: D114766. This patch does not depend on it.
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
2367 | Probably, but it does not help here. The interface is designed to evaluate constant value without construction of a node. ConstrainedFPCmpIntrinsic can be extracted from Call but checking intrinsic ID seems simpler. | |
llvm/lib/IR/Instructions.cpp | ||
4133 | I moved ConstantFold to include directory in dependency patch and put evaluatePredicate there. |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
2367 | FYI recently ICmpInst::compare() was added for the corresponding operation on ICmpInsts (in https://github.com/llvm/llvm-project/commit/25043c8276644e684f8d14cd4cadaa87a7e99b0e), so it might make sense to have the same method on FCmpInst. (No strong opinion on placement though.) |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
2365 | auto *FCmp I believe LLVM coding standards prefer to keep the * around with auto so things that are pointers are obvious. | |
2367 | The ConstrainedFPCmpIntrinsic is already being extracted from the call to get the predicate. So I thought it would make sense to get the signaling state from it as well and not pass the intrinsic ID at all. |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
2367 | Changed signature of evaluateCompare and fixed interface of ConstrainedFPCmpIntrinsic accordingly. |
My guess:
Removing the addition of the readnone attribute? It's a correctness issue. We don't have a solution to the performance issue in generated code, but correctness beats performance every time.
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1854 | Here. These two lines still need to be removed. Along with the comment, of course. |
llvm/lib/Analysis/ConstantFolding.cpp | ||
---|---|---|
1854 | Done in the separate patch: https://reviews.llvm.org/D115870 |
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
2028 | This was suggested in an earlier comment - can we add this next to: (and adjust the names/parameters for consistency) That can be a preliminary/NFC commit if I'm seeing it correctly. |
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
2028 | Previously such method existed but I was recommended to remove it: https://reviews.llvm.org/D110322?id=388475#3159399. Otherwise there is no problem to restore that implementation. |
I have no objections to this patch now. I'd pre-commit the baseline tests though, and let's see if there's consensus on where to house the helper function.
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
2028 | Ah, sorry I didn't see that earlier comment/revision. |
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
2028 | I think the integer change happened while this patch was in review. I'm fine with restoring the previous implementation to make this consistent with integer. |
Please pre-commit the baseline tests and FCmpInst::compare(), so we're left with only real logic changes and tests that show the change in behavior.
Hi,
This commit causes clang to crash when compiling the attached reproducer with the following compilation command:
clang -cc1 \ -emit-obj \ -target-feature +sse4.2 \ -frounding-math \ -O1 \ -std=gnu++17 \ -fsized-deallocation \ -o /tmp/repro.o -x c++ repro.cc
Please note that the crash only reproduces something like 60% of cases (just re-run a few times).
Please revert.
Thank you for the report. There was an issue with treatment of vector constants. Commit https://reviews.llvm.org/rG6982c38cb120 fixes it.
This still worries me. Are _analysis_ passes allowed to change the input IR? What if the caller decides to not do a fold after calling this function to see if a fold is possible? And why is the ebIgnore case different from the other two?
Constant folding won't add this ReadNone attribute when the constant folding code doesn't have an Instruction to alter. I don't know what to do about that.