kpn (Kevin P. Neal)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 20 2018, 9:31 AM (38 w, 2 d)

Recent Activity

Thu, Nov 8

kpn added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.
In D53157#1291964, @kpn wrote:

I don't expect anyone would need to switch between constrained and regular floating point at an instruction level of granularity.

The Standard allows for this (to some degree). E.g. FENV_ACCESS can be toggled between nested compound statements.

Thu, Nov 8, 11:46 AM
kpn added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Rather than having separate methods to create the constrained versions of the intrinsics, why not have a way to set the constrained state in the IR builder and have the regular CreateFAdd et. al. functions use that to automatically create the constrained intrinsic versions? This would correspond to how fast math flags are handled.

Thu, Nov 8, 11:30 AM
kpn updated subscribers of D53157: Teach the IRBuilder about constrained fadd and friends.
Thu, Nov 8, 11:27 AM

Wed, Nov 7

kpn added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Adding the usual suspects: @andrew.w.kaylor @craig.topper @uweigand @hfinkel

I don't see the benefit of this change. The intrinsics are good enough to be functional right now (minus a few that are in progress), so I'm not sure we need IRBuilder functions. Am I missing something?

Wed, Nov 7, 8:30 AM
kpn added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Should the builder's FastMathFlags setting apply to these calls? Is it worth adding an optional FMF parameter to these now, so we don't end up with duplicated API like we have for the regular FP binops?

Wed, Nov 7, 8:00 AM

Tue, Nov 6

kpn updated the diff for D53157: Teach the IRBuilder about constrained fadd and friends.

Update to have tests in this patch.

Tue, Nov 6, 12:14 PM
kpn updated the diff for D43515: More math intrinsics for conservative math handling.

Minor test changes.

Tue, Nov 6, 12:02 PM
kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Tue, Nov 6, 10:29 AM

Mon, Nov 5

kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Mon, Nov 5, 12:37 PM

Fri, Nov 2

kpn added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

The only thing we need to worry about is making sure that FP operations don't migrate across calls or other FP operations that would break these semantics. Using the intrinsics after inlining takes care of that.

I don't agree with this. This is changing the semantics if we choose to inline a function by converting some operations into constrained intrinsics. In other words, an executable will have different behavior if we choose to inline or not. That's not ok.

Well, yes and no. In a region defined as FENV_ACCESS off the user has granted the compiler explicit permission to ignore FP status bits and exception behavior. Depending on how we optimize these regions the FP status bits may come out differently and exceptions may or may not be raised, but the user has promised not to unmask exceptions or look at status bits in that region. So if that part of the behavior of the code changes depending on whether or not we inline that's OK because that's what the user signed up for. In that sense the behavior hasn't changed any more than it does between -O0 and -O3.

Fri, Nov 2, 12:20 PM
kpn added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.
In D43142#1285565, @kpn wrote:

If all optimizations including constant folding, or at least optimizations on floating point, are delayed until after inlining then there's no problem.

I'll add that this is a ton of work. A binary Instruction can't currently have two Constant operands. So, ConstantFolding is baked into the Instruction implementation right now. If I'm mistaken, someone please correct me.

Fri, Nov 2, 10:47 AM
kpn added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.
In D43142#1284190, @kpn wrote:

...

It's unclear to me how LTO (or other cross file inlining) would work here. I haven't given it much though until now. My knee-jerk reaction is that we shouldn't be inlining from a FENV_ACCESS=OFF to FENV_ACCESS=ON location.

I'm pretty sure that we decided that we couldn't (because once you inline the regular FP ops, there's no way to restrict their movement relative to the constrained intrinsics at the IR level).

I thought what we said was that if constrained FP intrinsics were used anywhere in a function then they had to be used everywhere in the function. So if you inline a function with FENV_ACCESS=ON into a function with FENV_ACCESS=OFF then the FP operations in the destination function need to be converted to constrained intrinsics, and if you inline a function with FENV_ACCESS=OFF into a function with FENV_ACCESS=ON then the FP operations in the function being inlined need to be converted. This conversion will have a detrimental effect on optimizations, so it would be nice to factor that into the inling cost, but at this point I'm mostly concerned about generating correct code.

Fri, Nov 2, 9:34 AM
kpn added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

I thought what we said was that if constrained FP intrinsics were used anywhere in a function then they had to be used everywhere in the function. So if you inline a function with FENV_ACCESS=ON into a function with FENV_ACCESS=OFF then the FP operations in the destination function need to be converted to constrained intrinsics, and if you inline a function with FENV_ACCESS=OFF into a function with FENV_ACCESS=ON then the FP operations in the function being inlined need to be converted. This conversion will have a detrimental effect on optimizations, so it would be nice to factor that into the inling cost, but at this point I'm mostly concerned about generating correct code.

I'm realizing that FENV_ACCESS is poorly designed.

Let's take this small example:

#include <fenv.h>
#include <stdio.h>

void bar();

#pragma STDC FENV_ACCESS ON
int main() {
  feenableexcept(FE_DIVBYZERO);
  bar();
  float x = 1.0/0.0;
  printf("main x=%lf\n", x);
}

#pragma STDC FENV_ACCESS OFF
void bar() {
  float x = 1.0/0.0;
  printf("bar x=%lf\n", x);
}

The FDiv in bar() would access the FP environment since main() enabled exceptions. But, since bar() is preceded with FENV_ACCESS=OFF, the Standard says that accessing the FP environment in bar() is undefined behavior. Since it's undefined behavior, the compiler can optimize as it wishes, including inlining bar() without modifying the FDiv.

Fri, Nov 2, 9:27 AM

Thu, Nov 1

kpn added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.
In D43142#1284190, @kpn wrote:

And couldn't we turn off constant folding in the IRBuilder when necessary?

I don't think that would work in all cases. E.g. a binary operator with two constant operands.

int main() {
  float z = 5.0/0.0;
  return z;
}

Well, not without a considerable effort to rework the IRBuilder at least. I'm fairly sure we can't produce an Instruction with two Constant operands right now.

Thu, Nov 1, 11:45 AM
kpn added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

Oh, of course FNeg will need to be emitted by the front end. And couldn't we turn off constant folding in the IRBuilder when necessary?

Thu, Nov 1, 10:49 AM
kpn added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

Isn't this pass needed since we don't have a way to prevent code movement between blocks using the pragma FENV_ACCESS ON and blocks that aren't? I suggest having it check for use of constrained intrinsics before making any changes.

Thu, Nov 1, 10:26 AM
kpn updated the diff for D43515: More math intrinsics for conservative math handling.

Address review comments.

Thu, Nov 1, 10:19 AM

Wed, Oct 31

kpn added a comment to D53932: [NFCI][FPEnv] Split constrained intrinsic tests.

FWIW, I like it.

Wed, Oct 31, 7:24 AM

Tue, Oct 30

kpn updated subscribers of D53424: Enable thread specific cl::opt values for multi-threaded support.

Personally, I'd like to see something where developer-only debug options show up in -help-hidden or similar for the correct executable: opt vs llc. Right now I'm pretty sure they don't. I don't have any opinion on how to make that happen, though.

Tue, Oct 30, 12:44 PM

Fri, Oct 19

kpn added a comment to D53411: [FPEnv] Add constrained CEIL/FLOOR/ROUND/TRUNC intrinsics.

While you're at it, shouldn't we also add ROUND and TRUNC to complete the set?

Fri, Oct 19, 5:56 AM

Thu, Oct 18

kpn added inline comments to D53216: [FPEnv] Add constrained intrinsics for MAXNUM and MINNUM.
Thu, Oct 18, 11:25 AM

Oct 12 2018

kpn updated the diff for D52839: Inform AST's UnaryOperator of FENV_ACCESS.

Upload the correct diff this time. Sorry about the confusion!

Oct 12 2018, 11:12 AM
kpn updated the diff for D52839: Inform AST's UnaryOperator of FENV_ACCESS.

Address review comments.

Oct 12 2018, 10:59 AM

Oct 11 2018

kpn added a comment to D52839: Inform AST's UnaryOperator of FENV_ACCESS.

I forgot to address the template question. I've spend the past 18 years doing SystemZ backend work on multiple {,JIT} compilers. I'm not really in a position to answer your question about how to handle getting these FP bits down into C++ templates. I hope that won't hold up getting FENV_ACCESS working for C compilations.

Oct 11 2018, 12:36 PM
kpn added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

The test case for this is in D52839, now.

Oct 11 2018, 12:03 PM
kpn updated the diff for D52839: Inform AST's UnaryOperator of FENV_ACCESS.

Address review comments.

Oct 11 2018, 12:02 PM
kpn created D53157: Teach the IRBuilder about constrained fadd and friends.
Oct 11 2018, 12:00 PM

Oct 9 2018

kpn updated the diff for D51372: FENV_ACCESS support for libm-style constrained intrinsics.

Update based on feedback to D52839: add missing AST (de)serialization support.

Oct 9 2018, 9:08 AM

Oct 4 2018

kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Oct 4 2018, 11:58 AM
kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Oct 4 2018, 10:54 AM
kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Oct 4 2018, 6:42 AM

Oct 3 2018

kpn added a comment to D43515: More math intrinsics for conservative math handling.

Ping

Oct 3 2018, 11:44 AM
kpn created D52839: Inform AST's UnaryOperator of FENV_ACCESS.
Oct 3 2018, 11:43 AM

Sep 18 2018

kpn updated the diff for D51372: FENV_ACCESS support for libm-style constrained intrinsics.

Correct obvious error with powi. Fix test and test both C and (partial) C++.

Sep 18 2018, 10:02 AM

Sep 17 2018

kpn updated the diff for D43515: More math intrinsics for conservative math handling.

Split out changes as requested. This diff is just the four new intrinsics. The fptoui pass and the change to frem will be later.

Sep 17 2018, 9:50 AM

Sep 10 2018

kpn added a comment to D43515: More math intrinsics for conservative math handling.

Adding new constrained instrinsics and adding the pass should be separate patches I think. Changing the syntax of frem should be another patch.

Sep 10 2018, 10:45 AM

Sep 4 2018

kpn updated the diff for D43515: More math intrinsics for conservative math handling.

Rebase. Ping.

Sep 4 2018, 9:17 AM

Aug 28 2018

kpn created D51372: FENV_ACCESS support for libm-style constrained intrinsics.
Aug 28 2018, 11:56 AM

Aug 27 2018

kpn added a comment to D51255: [WWW] Use https instead of http to fix page problems.
In D51255#1214424, @asl wrote:

I think we need to preserve the protocol. E.g. if the page is loaded via http we need to load css via http as well.

That sounds reasonable and is (afaik) implied by removing the hostname.
As mentioned before, I like the solution but I just don't know if that might cause problems somewhere else.

Aug 27 2018, 7:32 AM
kpn added a comment to D51255: [WWW] Use https instead of http to fix page problems.

Is there a good reason to have the hostname in the URL? Why not just link to "/Users.html" instead of "https://llvm.org/Users.html", for example? Using the hostname is only needed when linking to a page on a different server.

Aug 27 2018, 7:07 AM

Aug 24 2018

kpn added a comment to D50913: [FPEnv] Don't need copysign/fabs/fneg constrained intrinsics.

Did we decide that fast math flags can't be applied in the presence of constrained operations?

I haven't thought about this before today, but some of the flags we would want. E.g. reassoc/contract/afn.

In a pure trap-safe mode, contract/afn should be turned off. But, generally speaking, our users would prefer to see optimized code that misses some right-at-the-edge cases.

Aug 24 2018, 10:38 AM

Aug 20 2018

kpn added a comment to D50913: [FPEnv] Don't need copysign/fabs/fneg constrained intrinsics.

I just got done talking to one of our floating point math guys here.

Aug 20 2018, 11:55 AM
kpn added a comment to D50913: [FPEnv] Don't need copysign/fabs/fneg constrained intrinsics.

If the fsub that led to the FNEG node was not constrained in the first place then this seems like a valid transform.

Bah, sorry. My comment was ambiguous...

I meant that the FNEG->FSUB xform is "bad" in the context Ulrich proposed. For example:

The user writes a unary minus, -x, which is a bitwise operation and will never trap.

Aug 20 2018, 9:33 AM
kpn added a comment to D50913: [FPEnv] Don't need copysign/fabs/fneg constrained intrinsics.

I'm looking into the FNEG handling and found one bad xform in llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:

// Expand Y = FNEG(X) -> Y = SUB -0.0, X

That's not necessarily a show-stopper though. We could change this code to do a bitwise operation instead. That may even be faster than a FSUB, depending on the target of course.

Aug 20 2018, 7:43 AM
kpn added a comment to D50913: [FPEnv] Don't need copysign/fabs/fneg constrained intrinsics.

I do see that you wrote -0-x. Is there something special about the -0?

I understand this is to map -0 to +0 and vice versa.

User writes 0-x: Frontend maps to constrained FSUB
User writes -x: Frontend maps to FSUB

Yes, exactly.

Aug 20 2018, 6:53 AM

Aug 16 2018

kpn updated the diff for D43515: More math intrinsics for conservative math handling.

Rebase. Ping.

Aug 16 2018, 9:48 AM

Aug 14 2018

kpn committed rC339693.
Aug 14 2018, 10:07 AM
kpn committed rL339693.
Aug 14 2018, 10:07 AM
kpn closed D49865: Inform the AST of #pragma FENV_ACCESS use.
Aug 14 2018, 10:07 AM
kpn committed rC339691: Revert test commit.
Revert test commit
Aug 14 2018, 9:58 AM
kpn committed rL339691: Revert test commit.
Revert test commit
Aug 14 2018, 9:57 AM
kpn committed rC339690: Test commit.
Test commit
Aug 14 2018, 9:57 AM
kpn committed rL339690: Test commit.
Test commit
Aug 14 2018, 9:57 AM

Jul 31 2018

kpn updated the diff for D49865: Inform the AST of #pragma FENV_ACCESS use.

This includes the change to pass the annotation token when FENV_ACCESS is turned off.

Jul 31 2018, 11:37 AM
kpn updated the diff for D49865: Inform the AST of #pragma FENV_ACCESS use.

Replace accidentally lost comment.

Jul 31 2018, 9:44 AM
kpn updated the diff for D49865: Inform the AST of #pragma FENV_ACCESS use.

Thanks for the fast turnaround! I don't have commit access, can I get you to commit it for me? And can I get you to put my email address <kevin.neal@sas.com> in the commit message?

Jul 31 2018, 6:00 AM

Jul 30 2018

kpn added inline comments to D49865: Inform the AST of #pragma FENV_ACCESS use.
Jul 30 2018, 12:06 PM
kpn updated the diff for D49865: Inform the AST of #pragma FENV_ACCESS use.

Updated based on review.

Jul 30 2018, 12:04 PM

Jul 27 2018

kpn updated the diff for D43515: More math intrinsics for conservative math handling.

Delete a line accidentally left in.

Jul 27 2018, 10:02 AM

Jul 26 2018

kpn created D49865: Inform the AST of #pragma FENV_ACCESS use.
Jul 26 2018, 10:07 AM
kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Jul 26 2018, 9:59 AM
kpn updated the diff for D43515: More math intrinsics for conservative math handling.
Jul 26 2018, 9:16 AM
kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Jul 26 2018, 9:15 AM

Jun 20 2018

kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Jun 20 2018, 6:06 AM
kpn updated the diff for D43515: More math intrinsics for conservative math handling.
Jun 20 2018, 6:05 AM

May 1 2018

kpn updated the diff for D43515: More math intrinsics for conservative math handling.

Missed one place in the documentation that needed updating.

May 1 2018, 7:53 AM
kpn updated the diff for D43515: More math intrinsics for conservative math handling.

This new diff adds changing the default lowering of STRICT_FP_TO_UINT to not allow speculative execution. I've also eliminated the rounding metadata from instructions when it wasn't needed.

May 1 2018, 7:34 AM

Mar 6 2018

kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Mar 6 2018, 6:14 AM

Feb 26 2018

kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Feb 26 2018, 12:26 PM

Feb 21 2018

kpn added inline comments to D43515: More math intrinsics for conservative math handling.
Feb 21 2018, 12:31 PM

Feb 20 2018

kpn added a comment to D43515: More math intrinsics for conservative math handling.

At the request of my employer's legal department:
Copyright © 2018 SAS Institute Inc., Cary, NC, USA. All Rights Reserved.

Feb 20 2018, 9:48 AM
kpn created D43515: More math intrinsics for conservative math handling.
Feb 20 2018, 9:44 AM