[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

mibintc added a comment. The proposed llvm changes are now part of https://reviews.llvm.org/D62731 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___ cfe-commits mailing list

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL365339: Teach the IRBuilder about fadd and friends. (authored by kpn, committed by ). Herald added a project: LLVM. Chang

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added a comment. Thanks, this looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 207829. kpn added a comment. I was convinced in email that having the constrained intrinsics take fast math flags could be useful in some circumstances. The example that was given was that perhaps FMA contraction could be requested. So, when generating constrai

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn marked 2 inline comments as done. kpn added inline comments. Comment at: include/llvm/IR/IRBuilder.h:1324 + return CreateConstrainedFPBinOp(Intrinsic::experimental_constrained_fadd, + L, R, nullptr, Name); + rjmccall

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:259 +return DefaultConstrainedRounding.getValue(); + } + kpn wrote: > rjmccall wrote: > > Okay, so what are the invariants here now? It looks like, in order to > > enable constrained f

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn marked 3 inline comments as done. kpn added inline comments. Comment at: include/llvm/IR/IRBuilder.h:259 +return DefaultConstrainedRounding.getValue(); + } + rjmccall wrote: > Okay, so what are the invariants here now? It looks like, in order to enable

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:259 +return DefaultConstrainedRounding.getValue(); + } + Okay, so what are the invariants here now? It looks like, in order to enable constrained floating point, I need to also have se

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 207378. kpn added a comment. Address review comments. Eliminate ConstrainedFPIntrinsic's ebInvalid and rmInvalid enumeration values and replace them with use of the Optional<> class. Adjust the rest of the patch to take that into account. ConstrainedFPIntrinsic

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; kpn wrote: > rjmccall wrote: > > kpn wrote: > > > rjmccall wrote: > > > > Is it okay that `ebUnspecifie

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; rjmccall wrote: > kpn wrote: > > rjmccall wrote: > > > Is it okay that `ebUnspecified` and `ebInvalid` overl

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. I found out that some compilers, including SAS/C, will warn about enum overlaps like the one here under discussion. So I now believe John is correct and that eliminating that overlap is the right thing to do. Over the weekend I'll be thinking about whether or not to try and

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. Andy, can I get you to chime in? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; kpn wrote: > rjmccall wrote: > > Is it okay that `ebUnspecified` and `ebInvalid` overlap here? > I can

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn marked an inline comment as done. kpn added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; rjmccall wrote: > Is it okay that `ebUnspecified` and `ebInvalid` over

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; Is it okay that `ebUnspecified` and `ebInvalid` overlap here? Comment at: include/ll

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 205371. kpn added a comment. Add static methods to convert between a StringRef and the enums for RoundingMode or ExceptionBehavior. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 Files: include/llvm/IR/IRBui

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn marked an inline comment as done. kpn added inline comments. Comment at: include/llvm/IR/IRBuilder.h:1138 + +return MetadataAsValue::get(Context, RoundingMDS); + } rjmccall wrote: > kpn wrote: > > rjmccall wrote: > > > Huh? You build an `MDNode` that wr

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:1138 + +return MetadataAsValue::get(Context, RoundingMDS); + } kpn wrote: > rjmccall wrote: > > Huh? You build an `MDNode` that wraps an `MDString` and then immediately > > extract the

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added inline comments. Comment at: include/llvm/IR/IRBuilder.h:1138 + +return MetadataAsValue::get(Context, RoundingMDS); + } rjmccall wrote: > Huh? You build an `MDNode` that wraps an `MDString` and then immediately > extract the `MDString` from it an

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 205151. kpn marked 3 inline comments as done. kpn added a comment. Address review comments. Run the changes through clang-format. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 Files: include/llvm/IR/IRBuild

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:113 +CR_ToZero ///< This corresponds to "fpround.tozero". + }; + kpn wrote: > rjmccall wrote: > > Should these have "FP" in the name somewhere? And are they really > > IRBuilder-

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn marked an inline comment as done. kpn added inline comments. Comment at: include/llvm/IR/IRBuilder.h:113 +CR_ToZero ///< This corresponds to "fpround.tozero". + }; + rjmccall wrote: > Should these have "FP" in the name somewhere? And are they real

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:228 + /// Enable/Disable use of constrained floating point math + void setIsConstrainedFP(bool IsCon) { IsFPConstrained = IsCon; } + kpn wrote: > erichkeane wrote: > > kpn wrote: > > > kbar

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

mibintc added a comment. This works for me, I redid my patches adding fp-model options to work with this newest changeset, (but I haven't yet uploaded them.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 202987. kpn added a comment. Address the rest of the review comments, hopefully. I've lifted from mibintc's D62730 the enums for the exception behavior and rounding modes. The rest of that set of changes is better left to a sep

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

mibintc added a comment. In D53157#1525311 , @kpn wrote: > Oh, this ticket is not going to die from neglect. It is true that D43515 > is a higher priority, but I need this > IRBuilder work done as well. My department hea

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. Oh, this ticket is not going to die from neglect. It is true that D43515 is a higher priority, but I need this IRBuilder work done as well. My department head wanted it done by the end of _last_ year. It's not going to die. How about I mer

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

mibintc added inline comments. Comment at: include/llvm/IR/IRBuilder.h:234 + /// Set the exception handling to be used with constrained floating point + void setDefaultConstrainedExcept(MDNode *NewExcept) { +DefaultConstrainedExcept = NewExcept; kpn wrote

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. I wanted to get the API straight before working on documentation. Comment at: include/llvm/IR/IRBuilder.h:228 + /// Enable/Disable use of constrained floating point math + void setIsConstrainedFP(bool IsCon) { IsFPConstrained = IsCon; } +

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

lebedev.ri added a comment. I don't know which patch will proceed; but they weren't added already, this seems to be missing documentation changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 202436. kpn marked 4 inline comments as done. kpn added a comment. Address review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 Files: include/llvm/IR/IRBuilder.h unittests/IR/IRBuilderTest.cpp

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

mibintc added a comment. I've posted a small change to this patch here, https://reviews.llvm.org/D62730 and there's a patch to add clang fp options here, https://reviews.llvm.org/D62731 Comment at: include/llvm/IR/IRBuilder.h:234 + /// Set the exception handling to be used w

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

erichkeane added inline comments. Comment at: include/llvm/IR/IRBuilder.h:110 : Context(context), DefaultFPMathTag(FPMathTag), -DefaultOperandBundles(OpBundles) { +IsFPConstrained(false), DefaultConstrainedExcept(nullptr), +DefaultConstrainedRoundin

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

andrew.w.kaylor added inline comments. Comment at: include/llvm/IR/IRBuilder.h:234 + /// Set the exception handling to be used with constrained floating point + void setDefaultConstrainedExcept(MDNode *NewExcept) { +DefaultConstrainedExcept = NewExcept; I

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn marked an inline comment as done. kpn added a comment. I'm waiting for a signoff from at least one front-end guy. I hope the mode setting that allows us to keep front ends from having to touch every use of the IRBuilder is something that works for clang. But I haven't heard anything from @r

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kbarton added a comment. I think this looks straightforward, as long as people agree to have a separate CreateConstrained* version of these functions. I'm not qualified to weigh in on that as I don't know Clang at all and can't comment about the tradeoffs (although I think they have been well a

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. I emailed llvm-dev and cfe-dev on January 16th. The only responses I got back were of the don't care variety. For now it seems the constrained intrinsics will only be used by clang. @rsmith, does the direction of this patch seem reasonable for clang? Once Richard comments

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 176464. kpn added a comment. Address review comment: Shrink the diff by eliminating unneeded use of else. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 Files: include/llvm/IR/IRBuilder.h unittests/IR/IRBui

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added inline comments. Comment at: include/llvm/IR/IRBuilder.h:1244 } /// Copy fast-math-flags from an instruction rather than using the builder's I think you can forgo the `else {` in these functions since the if branch returns immediat

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In D53157#1316860 , @uweigand wrote: > In my reading of the standard text, there is no special provision for library > code. This means that in general, calling any library function while running > with nondefault trap

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn updated this revision to Diff 176432. kpn added a comment. I've changed the patch so that calls to CreateFAdd() et al will give you constrained intrinsics if they are enabled. This required adding functions to enable/disable constrained-as-default plus calls to deal with the rounding mode a

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

uweigand added a comment. In D53157#1309743 , @cameron.mcinally wrote: > Digressing a bit, but has anyone given thought to how this implementation > will play out with libraries? When running with traps enabled, libraries must > be compiled for trap-saf

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. Digressing a bit, but has anyone given thought to how this implementation will play out with libraries? When running with traps enabled, libraries must be compiled for trap-safety. E.g. optimizations on a lib's code could introduce a NaN or cause a trap that do

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

uweigand added a comment. In https://reviews.llvm.org/D53157#1305233, @kpn wrote: > In https://reviews.llvm.org/D53157#1304347, @uweigand wrote: > > > But given that there is still infrastructure missing in the IR optimizers, > > I also think that at least in the first implementation, we probabl

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. In https://reviews.llvm.org/D53157#1304347, @uweigand wrote: > But given that there is still infrastructure missing in the IR optimizers, I > also think that at least in the first implementation, we probably should go > with the original approach and just use constrained in

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In https://reviews.llvm.org/D53157#1304900, @andrew.w.kaylor wrote: > I think at this point we're all on the same page in this regard. I just > wanted to make sure we also understand why we're on that page. I still > believe it was the correct choice. Yup, we

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

andrew.w.kaylor added a comment. In https://reviews.llvm.org/D53157#1304895, @cameron.mcinally wrote: > The problem I was missing is when a FENV_ACCESS=OFF operation, like a FDIV, > is hoisted into a FENV_ACCESS=ON region. I see it now, but still think that > forcing FENV_ACCESS=OFF operations

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In https://reviews.llvm.org/D53157#1304880, @hfinkel wrote: > In https://reviews.llvm.org/D53157#1304873, @cameron.mcinally wrote: > > > I'd like to hear more about this. The fesetexcept(...) function and friends > > change the FPEnv state, which can change the

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In https://reviews.llvm.org/D53157#1304347, @uweigand wrote: > OK, let me try to expand on my point 3 above, which appears to have confused > everybody :-) Ah, that makes more sense. The problem I was missing is when a FENV_ACCESS=OFF operation, like a FDIV,

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

hfinkel added a comment. In https://reviews.llvm.org/D53157#1304873, @cameron.mcinally wrote: > In https://reviews.llvm.org/D53157#1304275, @kpn wrote: > > > In https://reviews.llvm.org/D53157#1303398, @cameron.mcinally wrote: > > > > > If we all agree upon that, then we simply have to treat the

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In https://reviews.llvm.org/D53157#1304275, @kpn wrote: > In https://reviews.llvm.org/D53157#1303398, @cameron.mcinally wrote: > > > If we all agree upon that, then we simply have to treat the functions that > > modify the FPEnv, e.g. fesetexcept(...), as barrie

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

uweigand added a comment. In https://reviews.llvm.org/D53157#1303193, @andrew.w.kaylor wrote: > I agree that it's preferable to re-use these existing options if possible. I > have some concerns that -ftrapping-math has a partial implementation in place > that doesn't seem to be well aligned wit

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

uweigand added a comment. OK, let me try to expand on my point 3 above, which appears to have confused everybody :-) First, let's distinguish two separate requirements: those on floating-point operations that explicitly run in regions with non-default control modes, and those on floating-point

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. In https://reviews.llvm.org/D53157#1303398, @cameron.mcinally wrote: > If we all agree upon that, then we simply have to treat the functions that > modify the FPEnv, e.g. fesetexcept(...), as barriers. That way it does not > matter if a FENV_ACCESS=OFF function is translate

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In https://reviews.llvm.org/D53157#1302724, @uweigand wrote: > A couple of comments on the previous discussion: > > 1. Instead of defining a new command line option, I'd prefer to use the > existing options -frounding-math and -ftrapping-math to set the default

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

andrew.w.kaylor added a comment. In https://reviews.llvm.org/D53157#1302724, @uweigand wrote: > A couple of comments on the previous discussion: > > 1. Instead of defining a new command line option, I'd prefer to use the > existing options -frounding-math and -ftrapping-math to set the default

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

uweigand added a comment. A couple of comments on the previous discussion: 1. Instead of defining a new command line option, I'd prefer to use the existing options -frounding-math and -ftrapping-math to set the default behavior of math operations w.r.t. rounding modes and exception status. (Fo

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

hfinkel added a comment. In https://reviews.llvm.org/D53157#1302159, @cameron.mcinally wrote: > In https://reviews.llvm.org/D53157#1301992, @hfinkel wrote: > > > > Just because FENV_ACCESS can be toggled on that granularity doesn't mean > > > we have to represent it that way. We've previously ag

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In https://reviews.llvm.org/D53157#1301992, @hfinkel wrote: > > Just because FENV_ACCESS can be toggled on that granularity doesn't mean we > > have to represent it that way. We've previously agreed (I think) that if > > FENV_ACCESS is enabled anywhere in a fun

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

hfinkel added a comment. In https://reviews.llvm.org/D53157#1301782, @cameron.mcinally wrote: > Fair enough. Just for conversation's sake, I was envisioning the FE support a > little differently... > > 1. Add a command line option, say `-ffpe_safe=[true|false]`, that toggles > FPEnv-safe code g

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

hfinkel added a comment. In https://reviews.llvm.org/D53157#1301991, @hfinkel wrote: > In https://reviews.llvm.org/D53157#1291977, @andrew.w.kaylor wrote: > > > Craig Topper also raised some concerns with me (in person, his desk is > > right by mine) about the potential effect this might have on

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

hfinkel added a comment. > Just because FENV_ACCESS can be toggled on that granularity doesn't mean we > have to represent it that way. We've previously agreed (I think) that if > FENV_ACCESS is enabled anywhere in a function we will want to use the > constrained intrinsics for all FP operation

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

hfinkel added a comment. In https://reviews.llvm.org/D53157#1291977, @andrew.w.kaylor wrote: > Craig Topper also raised some concerns with me (in person, his desk is right > by mine) about the potential effect this might have on code size. He tells me > that IRBuilder calls are intended to alwa

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. Fair enough. Just for conversation's sake, I was envisioning the FE support a little differently... 1. Add a command line option, say `-ffpe_safe=[true|false]`, that toggles FPEnv-safe code generation for a whole file. A `-ffpe_safe=true` would be equivalent t

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

andrew.w.kaylor added a comment. In https://reviews.llvm.org/D53157#1291978, @cameron.mcinally wrote: > In https://reviews.llvm.org/D53157#1291964, @kpn wrote: > > > I don't expect anyone would need to switch between constrained and regular > > floating point at an instruction level of granulari

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. In https://reviews.llvm.org/D53157#1291978, @cameron.mcinally wrote: > In https://reviews.llvm.org/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. > > > Th

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

cameron.mcinally added a comment. In https://reviews.llvm.org/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

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

andrew.w.kaylor added a comment. Craig Topper also raised some concerns with me (in person, his desk is right by mine) about the potential effect this might have on code size. He tells me that IRBuilder calls are intended to always be inlined and if we grow the implementation of these functions

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

kpn added a comment. In https://reviews.llvm.org/D53157#1290364, @andrew.w.kaylor wrote: > 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. func