Page MenuHomePhabricator

sepavloff (Serge Pavlov)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 13 2013, 7:26 AM (462 w, 1 d)

Recent Activity

Thu, Jan 13

sepavloff added a comment to D103395: PR45879: Keep evaluated expression in LValue object.

P1330R0 (https://wg21.link/P1330R0) allowed changing the active member of a union in constexpr expressions. To implement it the Constant Evaluator starting from
https://github.com/llvm/llvm-project/commit/31c69a3d6363463c08b86914c0c8cfc5c929c37e checks if an assignment changes active member of a union. This is why a change proposed for union only affects compilation of code that does not contain unions at all. The issue is in the assignment treatment.

Thu, Jan 13, 2:50 AM · Restricted Project

Thu, Dec 30

sepavloff accepted D115604: [Support] Expand `<CFGDIR>` as the base directory in configuration files..

LGTM

Thu, Dec 30, 9:55 AM · Restricted Project, Restricted Project
sepavloff committed rGecfd9196d5dd: [ConstantFolding] Use ICmpInst::Predicate instead of plain integer (authored by sepavloff).
[ConstantFolding] Use ICmpInst::Predicate instead of plain integer
Thu, Dec 30, 12:10 AM
sepavloff closed D116379: [ConstantFolding] Use ICmpInst::Predicate instead of plain integer.
Thu, Dec 30, 12:09 AM · Restricted Project

Wed, Dec 29

sepavloff added a comment to D116379: [ConstantFolding] Use ICmpInst::Predicate instead of plain integer.

Thanks!

Wed, Dec 29, 11:30 PM · Restricted Project
sepavloff added inline comments to D115604: [Support] Expand `<CFGDIR>` as the base directory in configuration files..
Wed, Dec 29, 11:29 PM · Restricted Project, Restricted Project
sepavloff added a comment to D115604: [Support] Expand `<CFGDIR>` as the base directory in configuration files..

The name of the patch could be more precise if you change response files to configuration files.

Wed, Dec 29, 9:47 AM · Restricted Project, Restricted Project
sepavloff added inline comments to D116168: [NFC] Method for evaluation of FCmpInst for constant operands.
Wed, Dec 29, 9:27 AM · Restricted Project
sepavloff requested review of D116379: [ConstantFolding] Use ICmpInst::Predicate instead of plain integer.
Wed, Dec 29, 9:25 AM · Restricted Project

Sat, Dec 25

sepavloff committed rGd86e2cc2e37c: [NFC] Method for evaluation of FCmpInst for constant operands (authored by sepavloff).
[NFC] Method for evaluation of FCmpInst for constant operands
Sat, Dec 25, 3:22 AM
sepavloff closed D116168: [NFC] Method for evaluation of FCmpInst for constant operands.
Sat, Dec 25, 3:22 AM · Restricted Project

Thu, Dec 23

sepavloff added a comment to D115604: [Support] Expand `<CFGDIR>` as the base directory in configuration files..

Thank you for the detailed explanation. Now I see how you are going to use this facility and think it worth implementation. A couple of questions.

Thu, Dec 23, 8:51 AM · Restricted Project, Restricted Project
sepavloff added a comment to D116168: [NFC] Method for evaluation of FCmpInst for constant operands.

Thanks!

Thu, Dec 23, 5:20 AM · Restricted Project

Wed, Dec 22

sepavloff added inline comments to D115604: [Support] Expand `<CFGDIR>` as the base directory in configuration files..
Wed, Dec 22, 9:05 AM · Restricted Project, Restricted Project
sepavloff updated the diff for D110322: [ConstantFolding] Fold constrained compare intrinsics.

FCmpInst::compare is moved to separate patch

Wed, Dec 22, 8:48 AM · Restricted Project
sepavloff requested review of D116168: [NFC] Method for evaluation of FCmpInst for constant operands.
Wed, Dec 22, 8:02 AM · Restricted Project
sepavloff committed rG185c80b89a25: [ConstantFolding] Tests for constrained compare intrinsics (authored by sepavloff).
[ConstantFolding] Tests for constrained compare intrinsics
Wed, Dec 22, 3:13 AM

Tue, Dec 21

sepavloff committed rG77b923d0dbe3: [ConstantFolding] Do not remove side effect from constrained functions (authored by sepavloff).
[ConstantFolding] Do not remove side effect from constrained functions
Tue, Dec 21, 10:47 PM
sepavloff closed D115870: [ConstantFolding] Do not remove side effect from constrained functions.
Tue, Dec 21, 10:47 PM · Restricted Project

Dec 20 2021

sepavloff updated the diff for D110322: [ConstantFolding] Fold constrained compare intrinsics.

Put back method of FCmpInst for constant comparison evaluation.

Dec 20 2021, 9:15 AM · Restricted Project
sepavloff added a comment to D115804: [CodeGen] use saturating FP casts when compiling with "no-strict-float-cast-overflow".

What's the plan for constrained intrinsics versions of these intrinsics? The IRBuilder calls for CreateFPToSI and CreateFPToUI are strict FP aware, but this new code isn't.

Not sure. cc'ing @kpn @sepavloff @andrew.w.kaylor
The saturating intrinsics implement non-standard behavior for C languages AFAIK, so we might want to warn if someone tries to use "-fno-strict-float-cast-overflow" and "-ffp-exception-behavior=strict" at the same time? Or we try to support that corner case by adding even more FP intrinsics?

Dec 20 2021, 3:59 AM · Restricted Project

Dec 16 2021

sepavloff added inline comments to D110322: [ConstantFolding] Fold constrained compare intrinsics.
Dec 16 2021, 6:42 AM · Restricted Project
sepavloff added inline comments to D110322: [ConstantFolding] Fold constrained compare intrinsics.
Dec 16 2021, 4:00 AM · Restricted Project
sepavloff requested review of D115870: [ConstantFolding] Do not remove side effect from constrained functions.
Dec 16 2021, 3:57 AM · Restricted Project

Dec 15 2021

sepavloff added a comment to D110322: [ConstantFolding] Fold constrained compare intrinsics.

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.

Dec 15 2021, 9:09 AM · Restricted Project
sepavloff added a comment to D110322: [ConstantFolding] Fold constrained compare intrinsics.

Is there anything I can do to make this patch acceptable?

Dec 15 2021, 8:36 AM · Restricted Project

Dec 14 2021

sepavloff accepted D111917: Print the sign of negative infinity.

LGTM.

Dec 14 2021, 7:19 AM · Restricted Project

Dec 10 2021

sepavloff accepted D115455: [RISCV] Remove FCSR from RISCVRegisterInfo..

Using fflags and frm separately is almost always more convenient than fcsr. The exception is probably the implementation of functions fegetenv and fesetenv, but in that case using both fflags and frm also works.

Dec 10 2021, 12:59 AM · Restricted Project

Dec 7 2021

sepavloff added a comment to D115035: Move header ConstantFold.h into include directory.

Thanks!

Dec 7 2021, 8:14 AM · Restricted Project

Dec 6 2021

sepavloff added inline comments to D110322: [ConstantFolding] Fold constrained compare intrinsics.
Dec 6 2021, 6:01 AM · Restricted Project
sepavloff added a comment to D114695: [SystemZ] Custom lowering of llvm.is_fpclass.

Thanks!

Dec 6 2021, 5:58 AM · Restricted Project
sepavloff updated the diff for D110322: [ConstantFolding] Fold constrained compare intrinsics.

Changed arguments of evaluateCompare and rebased

Dec 6 2021, 4:14 AM · Restricted Project

Dec 3 2021

sepavloff updated the diff for D115035: Move header ConstantFold.h into include directory.

Fixed header guard

Dec 3 2021, 3:01 AM · Restricted Project
sepavloff added a comment to D110322: [ConstantFolding] Fold constrained compare intrinsics.

Should the change that effects fadd, fma, etc. tests be moved to a different patch that is a pre-requisite of the compare change?

Dec 3 2021, 2:41 AM · Restricted Project
sepavloff updated the diff for D110322: [ConstantFolding] Fold constrained compare intrinsics.

Addressed reviewer's notes

Dec 3 2021, 2:33 AM · Restricted Project
sepavloff requested review of D115035: Move header ConstantFold.h into include directory.
Dec 3 2021, 2:31 AM · Restricted Project

Dec 2 2021

sepavloff added a comment to D114766: If constrained intrinsic is replaced, remove its side effect.

I think constrained FP intrinsics are basically in the same situation as math libcalls with errno. These get constant folded, and removal is handled via isMathLibCallNoop() in wouldInstructionBeTriviallyDead(). I would expected constrained FP intrinsics to basically work the same way. Of course, the implementation can be shared with the actual folding code, so for example wouldInstructionBeTriviallyDead() could check whether all intrinsic args are constant, if they are call the constrained FP folding function internally, and then say that it is trivially dead if it folds. (Or something more nuanced -- presumably there are cases where it's possible to determine the result but not drop, or maybe drop but not fold. Not familiar with the details here.)

Changing the infrastructure so that InstSimplify/ConstantFolding could report that the instruction is in fact dead (and not just the result known) is certainly also a possibility, but it's likely not the path of least resistance.

Dec 2 2021, 7:27 PM · Restricted Project
sepavloff added inline comments to D110579: [AMDGPU] Add two new intrinsics to control fp_trunc rounding mode.
Dec 2 2021, 7:02 PM · Restricted Project, Restricted Project

Dec 1 2021

sepavloff added inline comments to D110579: [AMDGPU] Add two new intrinsics to control fp_trunc rounding mode.
Dec 1 2021, 9:32 PM · Restricted Project, Restricted Project
sepavloff added inline comments to D110579: [AMDGPU] Add two new intrinsics to control fp_trunc rounding mode.
Dec 1 2021, 9:27 PM · Restricted Project, Restricted Project

Nov 30 2021

sepavloff added a comment to D114766: If constrained intrinsic is replaced, remove its side effect.

Let me summarize some points of the solution:

  1. Changing call site attributes is safe. It is equivalent to the creation of a new call site, with new set of attributes, and then replacing the old one with it, an ubiquitous operation in LLVM.
  2. The attribute adjustment can be made as soon as the possibility of constant folding is detected.
  3. The place where constant evaluation occurs is a subject of pipeline tuning. It is obvious that it should not run too early because the frontend already makes constant expression evaluation, so the constant evaluation in IR pipeline mainly evaluates constants formed by previous passes. Current point where InstSimplify is run probably is not the best, but it must be good enough.
Nov 30 2021, 9:48 PM · Restricted Project
sepavloff added a comment to D114766: If constrained intrinsic is replaced, remove its side effect.

Probably the pass should run earlier, it is hard to reason without analyzing practical cases. If it is indeed so, we could move the pass or run it several times. Tuning pipeline is not unique for constant folding only. But without ability to remove instructions many optimizations do not make sense.

But InstSimplify is used for analysis long before it is run for transformations. Other passes just call into InstSimplify and bypass InstSimplify's transformation pass machinery. Meaning, parts of InstSimplify do run earlier in the pipeline _already_.

Nov 30 2021, 9:40 AM · Restricted Project
sepavloff added a comment to D114766: If constrained intrinsic is replaced, remove its side effect.

Moving this logic to wouldInstructionBeTriviallyDead() does not seem right, because the decision about removing side effect comes from constant evaluation and not from the state of call object.

I'm sympathetic to this argument. If a transformation pass determines that a constrained intrinsic can be removed then it would be helpful if that pass could note that fact. This doesn't help us with analysis passes, though. And I'm a little worried about having to explicitly mark constrained intrinsics as removable since I don't know that we do that for any other instruction. If constrained intrinsics are the only time we do this then it seems error prone. Maybe I'm wrong?

The ability to remove side effect is vital for the implementation of floating point arithmetic. There are several possible optimization techniques, which depend on this possibility. For example, if FP operations in a function have attributes "round.dynamic" and "fpexcept.ignore" and we know that rounding mode is not changed in this function, we could remove side effect of all the operations, which can give performance of non-constrained operations. As we cannot mix ordinary and constrained operation in the same function, the only way is to control side effect.

We aren't saying that the ocean isn't full of water, we are only saying that you are looking at the pond while thinking it's an ocean.
We are talking about different things here. Fixing InstSimplify pass will not do any good, because it's effectively not used.

Nov 30 2021, 9:01 AM · Restricted Project
sepavloff added a comment to D114766: If constrained intrinsic is replaced, remove its side effect.

This is likely not going to be particularly useful, as the InstSimplifyPass is not really used outside of testing. To be generally applicable, this logic would have to be in wouldInstructionBeTriviallyDead().

Why? This pass is run when clang is executed. [...]

Yes, because we do happen to have a single InstSimplifyPass run at the very end of the pipeline (in https://github.com/llvm/llvm-project/blob/98dbcff19cfedb4e27d267310a76d616cd435447/llvm/lib/Passes/PassBuilderPipelines.cpp#L1194). However, mostly InstSimplify is only used as an analysis, with the most important consumer being InstCombine, which uses InstSimplify (the analysis, not the pass) and runs many times during the optimization pipeline. So while your code is indeed getting optimized, it is optimized very late, and you would likely see phase-ordering issues in a more practical example.

Nov 30 2021, 8:40 AM · Restricted Project
sepavloff added a reviewer for D114766: If constrained intrinsic is replaced, remove its side effect: andrew.w.kaylor.
Nov 30 2021, 8:18 AM · Restricted Project
sepavloff added a comment to D114766: If constrained intrinsic is replaced, remove its side effect.

Moving this logic to wouldInstructionBeTriviallyDead() does not seem right, because the decision about removing side effect comes from constant evaluation and not from the state of call object.

I'm sympathetic to this argument. If a transformation pass determines that a constrained intrinsic can be removed then it would be helpful if that pass could note that fact. This doesn't help us with analysis passes, though. And I'm a little worried about having to explicitly mark constrained intrinsics as removable since I don't know that we do that for any other instruction. If constrained intrinsics are the only time we do this then it seems error prone. Maybe I'm wrong?

Nov 30 2021, 8:18 AM · Restricted Project
sepavloff added a comment to D114766: If constrained intrinsic is replaced, remove its side effect.

This is likely not going to be particularly useful, as the InstSimplifyPass is not really used outside of testing. To be generally applicable, this logic would have to be in wouldInstructionBeTriviallyDead().

Nov 30 2021, 7:48 AM · Restricted Project

Nov 29 2021

sepavloff requested review of D114766: If constrained intrinsic is replaced, remove its side effect.
Nov 29 2021, 9:21 PM · Restricted Project
sepavloff added a comment to D110322: [ConstantFolding] Fold constrained compare intrinsics.

Any feedback?

Nov 29 2021, 1:51 AM · Restricted Project
sepavloff requested review of D114695: [SystemZ] Custom lowering of llvm.is_fpclass.
Nov 29 2021, 1:50 AM · Restricted Project

Nov 25 2021

sepavloff updated the diff for D83036: [X86][FPEnv] Lowering of {get,set,reset}_fpmode.

Update patch because dependency has been updated

Nov 25 2021, 9:11 AM · Restricted Project
sepavloff updated the diff for D82525: [FPEnv] Intrinsics for access to FP control modes.

Updated patch

Nov 25 2021, 8:53 AM · Restricted Project
sepavloff updated the summary of D82525: [FPEnv] Intrinsics for access to FP control modes.
Nov 25 2021, 8:50 AM · Restricted Project

Nov 22 2021

sepavloff added a comment to D113908: [PowerPC] Support of ppc_fp128 in lowering of llvm.is_fpclass.

Thanks!

Nov 22 2021, 4:25 AM · Restricted Project

Nov 19 2021

sepavloff updated the diff for D110322: [ConstantFolding] Fold constrained compare intrinsics.

Move attribute modification to InstSimplify pass

Nov 19 2021, 5:22 AM · Restricted Project

Nov 18 2021

sepavloff retitled D113908: [PowerPC] Support of ppc_fp128 in lowering of llvm.is_fpclass from [PowerPC] Custom lowering of llvm.is_fpclass to [PowerPC] Support of ppc_fp128 in lowering of llvm.is_fpclass.
Nov 18 2021, 9:11 PM · Restricted Project
sepavloff updated the diff for D113908: [PowerPC] Support of ppc_fp128 in lowering of llvm.is_fpclass.

Updated patch

Nov 18 2021, 9:09 PM · Restricted Project

Nov 15 2021

sepavloff requested review of D113908: [PowerPC] Support of ppc_fp128 in lowering of llvm.is_fpclass.
Nov 15 2021, 9:04 AM · Restricted Project

Nov 12 2021

sepavloff added a comment to D103395: PR45879: Keep evaluated expression in LValue object.

Strange, I see that it cannot be compiled neither by gcc nor by clang: https://godbolt.org/z/1dY9Gs6zM. Do I miss something?

Sorry, should've been more specific. Try in C++20 mode: https://godbolt.org/z/4v8b3nsET
I think the difference might be due to P1331R2, but I'm not sure.

Nov 12 2021, 5:41 AM · Restricted Project
sepavloff updated the diff for D103395: PR45879: Keep evaluated expression in LValue object.

Update the patch

Nov 12 2021, 5:33 AM · Restricted Project

Nov 11 2021

sepavloff committed rG3057e850b88e: [X86] Preserve FPSW when popping x87 stack (authored by sepavloff).
[X86] Preserve FPSW when popping x87 stack
Nov 11 2021, 9:56 PM
sepavloff closed D113335: [X86] Preserve FPSW when popping x87 stack.
Nov 11 2021, 9:56 PM · Restricted Project
sepavloff added inline comments to D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.
Nov 11 2021, 8:29 AM · Restricted Project
sepavloff added inline comments to D112025: Intrinsic for checking floating point class.
Nov 11 2021, 8:17 AM · Restricted Project
sepavloff added inline comments to D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.
Nov 11 2021, 2:35 AM · Restricted Project

Nov 10 2021

sepavloff added inline comments to D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.
Nov 10 2021, 7:34 AM · Restricted Project
sepavloff updated the diff for D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.

Remove support of f32 and f64

Nov 10 2021, 6:40 AM · Restricted Project

Nov 9 2021

sepavloff added inline comments to D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.
Nov 9 2021, 9:25 AM · Restricted Project
sepavloff updated the diff for D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.

Address reviewer's notes.

Nov 9 2021, 9:04 AM · Restricted Project

Nov 8 2021

sepavloff added a comment to D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.

This patch was tested using runtime tests from https://reviews.llvm.org/D112933 and a patch for clang https://reviews.llvm.org/D112932.

Nov 8 2021, 9:13 AM · Restricted Project
sepavloff requested review of D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80.
Nov 8 2021, 9:07 AM · Restricted Project
sepavloff updated the diff for D112933: [WIP] Tests for additional builtin classification functions.

Restore support of long double

Nov 8 2021, 8:49 AM

Nov 7 2021

sepavloff added a comment to D113335: [X86] Preserve FPSW when popping x87 stack.

Thanks!

Nov 7 2021, 7:37 AM · Restricted Project
sepavloff added inline comments to D113335: [X86] Preserve FPSW when popping x87 stack.
Nov 7 2021, 6:32 AM · Restricted Project
sepavloff updated the diff for D113335: [X86] Preserve FPSW when popping x87 stack.

Update patch

Nov 7 2021, 6:30 AM · Restricted Project

Nov 6 2021

sepavloff requested review of D113335: [X86] Preserve FPSW when popping x87 stack.
Nov 6 2021, 1:19 AM · Restricted Project

Nov 3 2021

sepavloff added a comment to D103395: PR45879: Keep evaluated expression in LValue object.

Thank you for feedback!

Nov 3 2021, 4:16 AM · Restricted Project
sepavloff updated the diff for D103395: PR45879: Keep evaluated expression in LValue object.

Updated patch

Nov 3 2021, 3:37 AM · Restricted Project

Nov 1 2021

sepavloff added a comment to D112025: Intrinsic for checking floating point class.

This patch was tested using a patches for clang https://reviews.llvm.org/D112932 and test-suite https://reviews.llvm.org/D112933. Clang build with the patch were used to compile llvm-test-suite/SingleSource/UnitTests/Float/classify.c. Running the obtained executable executes the unit tests.

Nov 1 2021, 8:44 AM · Restricted Project
sepavloff requested review of D112933: [WIP] Tests for additional builtin classification functions.
Nov 1 2021, 8:33 AM
sepavloff requested review of D112932: [WIP] Use llvm.is_fpclass to implement FP classification functions.
Nov 1 2021, 8:31 AM · Restricted Project
sepavloff updated the diff for D112025: Intrinsic for checking floating point class.

Updated patch

Nov 1 2021, 7:17 AM · Restricted Project

Oct 22 2021

sepavloff added inline comments to D112025: Intrinsic for checking floating point class.
Oct 22 2021, 12:02 AM · Restricted Project

Oct 21 2021

sepavloff updated the diff for D112025: Intrinsic for checking floating point class.

Make test specification more flexible

Oct 21 2021, 11:04 PM · Restricted Project

Oct 18 2021

sepavloff added a comment to D112025: Intrinsic for checking floating point class.

The patch was tested in runtime using extended version of tests https://reviews.llvm.org/D106804 and temporary clang patch that implements classification builtins by calls to llvm.is.fclass.

Oct 18 2021, 11:53 AM · Restricted Project
sepavloff requested review of D112025: Intrinsic for checking floating point class.
Oct 18 2021, 11:49 AM · Restricted Project

Oct 17 2021

sepavloff committed rT22d9df15f561: Fixed tests for FP classification intrinsics (authored by sepavloff).
Fixed tests for FP classification intrinsics
Oct 17 2021, 10:04 PM

Oct 6 2021

sepavloff added inline comments to D111085: [FPEnv][InstSimplify] Fold constrained X + -0.0 ==> X.
Oct 6 2021, 10:16 AM · Restricted Project

Oct 4 2021

sepavloff accepted D111085: [FPEnv][InstSimplify] Fold constrained X + -0.0 ==> X.

LGTM.

Oct 4 2021, 10:49 PM · Restricted Project

Oct 1 2021

sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

Ouch, this is a long thread.

If I understand correctly, an important missing feature in Alive2 is support for multiple rounding modes. I didn't implement that so far mostly because I was under the impression that rounding modes was still a working in progress thing. Is this true or is it mostly settled in stone?

Oct 1 2021, 6:26 AM · Restricted Project

Sep 30 2021

sepavloff updated the diff for D110322: [ConstantFolding] Fold constrained compare intrinsics.

Set ReadNone for all intrinsics if they can be evaluated without raising signals

Sep 30 2021, 11:38 PM · Restricted Project
sepavloff added inline comments to D110322: [ConstantFolding] Fold constrained compare intrinsics.
Sep 30 2021, 11:31 PM · Restricted Project
sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

Here is the sample program: https://godbolt.org/z/ssYs6ez91
Hardware converts SNaN + 0 into QNaN.

This is cherry-picking the example to try to prove your point; gcc does not behave as you are suggesting in general. Here is a counter-example based on the first transform (fadd X, -0.0) affected by this patch:
https://godbolt.org/z/qj9Meavnd

In this case no add instruction is generated, so it does not demonstrate hardware behavior. It is (incorrect) constant folding. ICC does not do such transformation. Could you please file a bug in GCC bug tracker?

Apologies to this patch review for being off-topic again, but the answer is no.
I think you know that's not a standalone folding bug. It's intentional behavior in both compilers which you can easily show with a number of similar examples...
https://godbolt.org/z/fT1j8YPK6

Sep 30 2021, 11:18 PM · Restricted Project
sepavloff updated the diff for D110322: [ConstantFolding] Fold constrained compare intrinsics.

Added tests with "maytrap"

Sep 30 2021, 11:58 AM · Restricted Project
sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

Here is the sample program: https://godbolt.org/z/ssYs6ez91
Hardware converts SNaN + 0 into QNaN.

This is cherry-picking the example to try to prove your point; gcc does not behave as you are suggesting in general. Here is a counter-example based on the first transform (fadd X, -0.0) affected by this patch:
https://godbolt.org/z/qj9Meavnd

Sep 30 2021, 10:06 AM · Restricted Project
sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

fadd X, -0 ==> X is *NOT* a miscompile, at least given the current LLVM IR semantics: https://alive2.llvm.org/ce/z/TuTiSQ
I would personally strongly suggest to not reason about semantics via hand-waving, but to actually model them in alive2, if it isn't already.
Honestly, i'm quite worried that this is repeating the same approach as in isnan threads.
Some might interpret it as being dismissive/intentionally ignoring documented semantics.

Here is the sample program: https://godbolt.org/z/ssYs6ez91
Hardware converts SNaN + 0 into QNaN.

Could you please write a little longer arguments?
Your point being? Pick one of:

  1. the optimization is incorrect as per the llvm langref
  2. the optimization is correct as per the llvm langref, which is itself incorrect
  3. ???

Which one is it?

Sep 30 2021, 7:57 AM · Restricted Project
sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

fadd X, -0 ==> X is *NOT* a miscompile, at least given the current LLVM IR semantics: https://alive2.llvm.org/ce/z/TuTiSQ
I would personally strongly suggest to not reason about semantics via hand-waving, but to actually model them in alive2, if it isn't already.
Honestly, i'm quite worried that this is repeating the same approach as in isnan threads.
Some might interpret it as being dismissive/intentionally ignoring documented semantics.

Sep 30 2021, 6:44 AM · Restricted Project
sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

Changing the way the existing transforms check for FMF.noNaNs() sounds like a different ticket that needs to be done before this one can progress. Unless I misunderstood?

I know nothing about such change.

Your proposed changes around SNaN will affect the non-constrained cases.

I think the optimization fadd X, -0 ==> X in general case (not FMF.noNaNs()) is incorrect. The obtained values must be identical to what would produce hardware, otherwise it is not an optimization. If you think that such change deserves a separate patch, no problem. It seems to me that this patch could establish correct folding even if it changes non-constrained operation as well.

I really don't think we should be changing how LLVM handles the ebIgnore cases. LLVM currently treats SNaN and QNaN as the same except where it doesn't, and I believe we should keep that behavior unchanged when ignoring exceptions. At the very least we shouldn't be disabling any currently enabled optimizations when ignoring exceptions. @lebedev.ri or @jcranmer ?

Sep 30 2021, 3:14 AM · Restricted Project

Sep 29 2021

sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

Changing the way the existing transforms check for FMF.noNaNs() sounds like a different ticket that needs to be done before this one can progress. Unless I misunderstood?

I know nothing about such change.

Your proposed changes around SNaN will affect the non-constrained cases.

Sep 29 2021, 10:47 AM · Restricted Project
sepavloff added a comment to D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd.

Changing the way the existing transforms check for FMF.noNaNs() sounds like a different ticket that needs to be done before this one can progress. Unless I misunderstood?

Sep 29 2021, 10:12 AM · Restricted Project