Page MenuHomePhabricator

Teach the IRBuilder about constrained fadd and friends
ClosedPublic

Authored by kpn on Oct 11 2018, 11:59 AM.

Details

Summary

The IRBuilder has calls to create floating point instructions like fadd. It does not have calls to create constrained versions of them. This patch adds support for constrained creation of fadd, fsub, fmul, fdiv, and frem.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 does not exist in the code itself.

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 settings is undefined behavior, unless the library was itself compiled with FENV_ACCESS ON. There does not even appear to be any requirement for the C standard library functions to have been compiled with FENV_ACCESS ON, as far as I can see ...

kpn updated this revision to Diff 176432.Dec 3 2018, 10:51 AM

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 and exception handling for contrained intrinsics.

I've left the independent create constrained functions to make it dead easy to move them out of the header and avoid inlining bloat.

I'm not dealing with any fast math flags or metadata currently. I'm not sure they make sense in a constrained context, or at least I haven't seen anyone argue they must be in place for an initial implementation.

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 settings is undefined behavior, unless the library was itself compiled with FENV_ACCESS ON. There does not even appear to be any requirement for the C standard library functions to have been compiled with FENV_ACCESS ON, as far as I can see ...

I'll take this conversation to llvm-dev in the near future. I don't want to detract from Kevin's work.

Apologies for highjacking this Diff, Kevin.

include/llvm/IR/IRBuilder.h
1243

I think you can forgo the else { in these functions since the if branch returns immediately.

kpn updated this revision to Diff 176464.Dec 3 2018, 1:13 PM
kpn marked an inline comment as done.

Address review comment: Shrink the diff by eliminating unneeded use of else.

kpn added a comment.Feb 5 2019, 9:57 AM

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 is there anything else needing us to delay here?

kpn added a reviewer: rsmith.May 10 2019, 10:18 AM
kpn added a subscriber: kbarton.May 10 2019, 12:33 PM

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 articulated in the discussion). From what I can see, that is the only thing blocking this from getting approved (unless there is something else I missed while reading the discussion).

The only other comment I have is there was some very good description about the intention here from @uweigand, @cameron.mcinally and @andrew.w.kaylor and @kpn. I think it would be good if that discussion is extracted from this review and put somewhere (perhaps the language ref) to indicate precisely what the semantics are we are trying to achieve with FENV_ACCESS ON/OFF.

include/llvm/IR/IRBuilder.h
228

This is a minor quibble, but the method is setIsConstrainedFP, while the member is IsFPConstrained.
I'm not sure if that is intentionally different, or an oversight.

kpn marked an inline comment as done.May 15 2019, 1:12 PM

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 @rsmith or any other front-end person.

include/llvm/IR/IRBuilder.h
228

Yeah, that's an oversight. Fixed.

include/llvm/IR/IRBuilder.h
234

I think it would be better to add some enumerated type to describe the exception semantic and rounding modes. The MDNode is an implementation detail and an awkward one for the front end to have to deal with.

erichkeane added inline comments.
include/llvm/IR/IRBuilder.h
110

Instead of adding these here, make these inline initializers on lines 100-102.

228

IS this fixed?

1249

Why set the last 2 to nullptr when you have defaults for these?

1259

The last 2 parameters are never actually used except in the test. Are these really important to have if they are never used in source?

1408

All these CreateConstrainedXXX should be distilled down to a single function that takes the intrinsic as a parameter.

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

include/llvm/IR/IRBuilder.h
234

I posted a patch showing the rounding and exception information being passed as enumeration not MDNode. I've uploaded the patch here, https://reviews.llvm.org/D62730

kpn updated this revision to Diff 202436.May 31 2019, 7:54 AM
kpn marked 4 inline comments as done.

Address review comments.

I don't know which patch will proceed; but they weren't added already, this seems to be missing documentation changes.

kpn added a comment.May 31 2019, 8:04 AM

I wanted to get the API straight before working on documentation.

include/llvm/IR/IRBuilder.h
228

In my working copy, yes.

234

It's a good idea. D62730 beat me to the punch.

1249

Eh, no really good reason.

1259

They're part of the intrinsics. It seems like there should be a way to emit them as part of the instruction when creating the instruction.

1408

That moves us further down the road towards having modes. I haven't seen a front-end guy sign off on it yet, but it seems like a good idea.

mibintc added inline comments.May 31 2019, 8:17 AM
include/llvm/IR/IRBuilder.h
234

BTW I created FPState.h because if I put the enumeration types in IRBuilder it dragged in too many include files in clang and required other random source changes for disambigutation. Anyway I don't need to comandeer your patch -- I don't want it to die from neglect. I can close mine?

kpn added a comment.May 31 2019, 9:52 AM

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 merge your changes into this ticket and we continue work over here? There is the issue of the documentation that lebedev.ri asked you to write. Can I talk you into putting that together and sending it to me <kevin.neal@sas.com>? I'll work on the documentation that I was asked to write. Between the two of us we should be in pretty good shape. Does that work for you?

I am still waiting for feedback from an actual consumer of the IRBuilder who will be using this new functionality. If someone clang-side could chime in on this ticket I'd very much appreciate it.

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 head wanted it done by the end of _last_ year. It's not going to die.

How about I merge your changes into this ticket and we continue work over here? There is the issue of the documentation that lebedev.ri asked you to write. Can I talk you into putting that together and sending it to me <kevin.neal@sas.com>? I'll work on the documentation that I was asked to write. Between the two of us we should be in pretty good shape. Does that work for you?

Yes I'll do that. Thanks.

I am still waiting for feedback from an actual consumer of the IRBuilder who will be using this new functionality. If someone clang-side could chime in on this ticket I'd very much appreciate it.

I wrote a clang patch that works with your IRBuilder modifications. I also checked with the Intel fortran team and they think this interface will be workable for them too. From Intel perspective it's +1.

kpn updated this revision to Diff 202987.Jun 4 2019, 11:04 AM

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 separate ticket after all.

Since I now have those requested enums I've changed the relevant public functions to take them instead of an MDNode*.

There is no standalone or comprehensive IRBuilder documentation that I could find. I hope the references in comments back to the LangRef are good enough. I used the exact language from it to make it easier to reference said doc.

simoll added a subscriber: simoll.Jun 17 2019, 5:29 AM

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.)

rjmccall added inline comments.Jun 17 2019, 9:50 AM
include/llvm/IR/IRBuilder.h
114

Should these have "FP" in the name somewhere? And are they really IRBuilder-specific concepts, as opposed to something that should be declared as part of the interface for working with these intrinsics?

Also, I believe we can use explicit underlying types now in LLVM; it'd be nice if we didn't make IRBuilder unnecessarily large.

228

Maybe this should be more explicit about what exactly it does? Specifically, it changes the behavior of the CreateF<Op> methods so that they use constrained intrinsics instead of the standard instructions.

231

This seems unnecessary.

1043

Huh? You build an MDNode that wraps an MDString and then immediately extract the MDString from it and drop the MDNode?

I think you should just have a function somewhere that returns the correct metadata string for a ConstrainedRoundingKind, and then the code here is much more obvious. Maybe this can be wherever you declare the enums. You can also have a function that goes the other way and returns an Optional<ConstrainedRoundingKind>.

Function name should include FP.

Same points apply to the next function.

kpn marked an inline comment as done.Jun 17 2019, 10:10 AM
kpn added inline comments.
include/llvm/IR/IRBuilder.h
114

Would it be better to use the RoundingMode and ExceptionBehavior enums in the ConstrainedFPIntrinsic class? These enums->strings here get turned back into those IntrinsicInst.h enums eventually anyway. But that means pulling in yet more headers in IRBuilder.h.

I admit I'm not sure what you mean with your second paragraph. Is that a way of saying that, for example, the relevant IntrinsicInst.h enums should be used instead?

rjmccall added inline comments.Jun 17 2019, 11:00 AM
include/llvm/IR/IRBuilder.h
114

I think it's IRBuilder.h's fate to pull in the majority of the headers in IR, but even if we want to avoid that, duplicating the enums seems like a step too far. If there's too much code in the header declaring that intrinsic, you should extract a smaller header that just declares the enums and their string conversions.

The second paragraph is asking for these enums to be specifically constrained to uint8_t so that we aren't wasting a ton of memory everywhere we store them. IRBuilder doesn't have strong size constraints, but it'd still be nice if all these accumulated features didn't make it hundreds of bytes larger than it needs to be.

kpn updated this revision to Diff 205151.Jun 17 2019, 12:44 PM
kpn marked 3 inline comments as done.

Address review comments.

Run the changes through clang-format.

include/llvm/IR/IRBuilder.h
1043

Well, when you put it that way, your proposed change does result in much simpler code.

The resulting code isn't that much more than the switch. I hope it is now easy enough to read. These functions are private to the class and just exist to be helpers. I don't see a need to put the switch table into a different function, but I'm willing to be convinced.

rjmccall added inline comments.Jun 17 2019, 12:55 PM
include/llvm/IR/IRBuilder.h
1043

I feel like having the enum->string mapping available somewhere independent of IRBuilder will probably be useful to someone, and it nicely separates concerns in any case . You could just make it a static method on ConstrainedFPIntrinsic along with the reverse operation.

kpn marked an inline comment as done.Jun 17 2019, 1:06 PM
kpn added inline comments.
include/llvm/IR/IRBuilder.h
1043

Ok, will do.

kpn updated this revision to Diff 205371.Jun 18 2019, 9:06 AM

Add static methods to convert between a StringRef and the enums for RoundingMode or ExceptionBehavior.

rjmccall added inline comments.Jun 18 2019, 10:28 AM
include/llvm/IR/IntrinsicInst.h
235 ↗(On Diff #205371)

Is it okay that ebUnspecified and ebInvalid overlap here?

245 ↗(On Diff #205371)

You should document the behavior of the StrTo... functions when the string is unexpected and the ...ToStr functions when the enum is invalid/unspecified.

kpn marked an inline comment as done.Jun 18 2019, 10:54 AM
kpn added inline comments.
include/llvm/IR/IntrinsicInst.h
235 ↗(On Diff #205371)

I can think of a couple of alternatives. If they don't overlap then we have to go back and sweep the source to make sure that ebUnspecified is always handled in all cases that currently handle ebInvalid. And in the future nobody is allowed to check in source that doesn't handle both.

Or, we could drop ebUnspecified, but then ebInvalid would have a valid meaning to the IRBuilder interface. That looks like a bug even if it works properly.

Generally, adding eb/rmUnspecified but having them overlap the invalid cases seems to me to be the best option.

rjmccall added inline comments.Jun 18 2019, 12:15 PM
include/llvm/IR/IntrinsicInst.h
235 ↗(On Diff #205371)

What is the purpose of ebInvalid? Is this a valid argument to the intrinsic, or is it there so that getExceptionBehavior() has something to return if it doesn't recognize the argument? For the latter, maybe it would be better to just assert that the argument was one of the recognized possibilities; we don't generally expect LLVM to silently propagate ill-formed IR. Or if it's valid to not provide exception behavior to the intrinsic, maybe that's the same as ebUnspecified, or maybe the accessor should return Optional<ExceptionBehavior>.

Andy, can I get you to chime in?

kpn added a comment.Jun 20 2019, 8:45 AM

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 find an alternative, but I'll be out of cell phone range until late Sunday evening.

kpn added inline comments.Jun 26 2019, 11:20 AM
include/llvm/IR/IntrinsicInst.h
235 ↗(On Diff #205371)

The current code is in ConstrainedFPIntrinsic::getExceptionBehavior(), which returns ebInvalid if the metadata is missing or somehow wrong (wrong type, invalid string, etc). This is then used by the IR verifier which fails with an Assert() if it gets ebInvalid. So we aren't propagating ill-formed IR.

The intrinsic requires the metadata, and it requires the metadata be valid. But I don't want to clutter up IRBuilder's consumer's code. That's why I wanted the API to allow those arguments to not be given.

One idea of mine is to have ebInvalid be valid when given to the IRBuilder, and the meaning would be "this value is unspecified". But that seems weird.

Maybe there's another way.

rjmccall added inline comments.Jun 26 2019, 11:44 AM
include/llvm/IR/IntrinsicInst.h
235 ↗(On Diff #205371)

The current code is in ConstrainedFPIntrinsic::getExceptionBehavior(), which returns ebInvalid if the metadata is missing or somehow wrong (wrong type, invalid string, etc). This is then used by the IR verifier which fails with an Assert() if it gets ebInvalid. So we aren't propagating ill-formed IR.

Okay, in that case, I don't understand why this accessor shouldn't just assume that it's working on valid IR and not be able to return ebInvalid.

The intrinsic requires the metadata, and it requires the metadata be valid. But I don't want to clutter up IRBuilder's consumer's code. That's why I wanted the API to allow those arguments to not be given.

If you want the IRBuilder factories to be able to not take a value (if there's some defaulting scheme that's more complicated than you can express with a default argument), you can have it take an Optional<Whatever>, and then people can just pass in None; and then you don't need to muddy up the enum with a case that isn't really a valid alternative.

kpn updated this revision to Diff 207378.Jul 1 2019, 10:53 AM

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 now supports string <--> enum conversions.

rjmccall added inline comments.Jul 1 2019, 11:24 AM
include/llvm/IR/IRBuilder.h
259

Okay, so what are the invariants here now? It looks like, in order to enable constrained floating point, I need to also have set the default exception behavior and rounding mode. That should at least be documented, but maybe a better approach would be to require these to be passed in when enabling constrained FP.

1238

FPMD is dropped in this case; I don't know if that's intentional.

1437

It looks like nothing in IRBuilder ever passes these arguments. Are you just anticipating that someone might want to call this directly?

kpn marked 3 inline comments as done.Jul 1 2019, 12:11 PM
kpn added inline comments.
include/llvm/IR/IRBuilder.h
259

The IRBuilder constructor sets the defaults to ebStrict and rmDynamic, but leaves strict mode off. So if you only turn on strict mode you get the strictest settings.

This implies that it isn't possible to trigger this assertion. Which is true unless you have some form of memory overwrite or someone comes along later and puts in a bad IRBuilder change.

The more compiler work I've done over the years the more sanity checks I've gotten into the habit of adding.

1238

You can see that I'm on the fence here. I'm not sure that mixing fast math with constrained math makes sense. So CreateConstrainedFPBinOp() can take an instruction for copying fast math flags, but it doesn't do anything with this instruction. And the FPMD is simply dropped in the non-*FMF() methods.

If we do decide later to support mixing constrained and fast math then we won't have to change any APIs. Until then it takes the conservative route and drops the info.

1437

Correct. And I wrote tests for it in IRBuilderTest.cpp.

rjmccall added inline comments.Jul 1 2019, 1:45 PM
include/llvm/IR/IRBuilder.h
259

Okay, that scheme makes sense, but I think sanity-checking that there hasn't been an arbitrary memory smasher is a bit much. Using Optional when the value can't actually be missing just makes the code more confusing for readers and maintainers; please just leave these non-optional.

1238

Okay. That should probably be mentioned, at least in the documentation for CreateConstrainedFPBinOp. Should you make an overload of the latter which takes an MDNode* as the final argument, for parallelism/completeness?

1437

Okay. So people who want to pass these explicitly just can't use the convenience methods? That seems like a reasonable compromise.

kpn marked 2 inline comments as done.Jul 3 2019, 10:24 AM
kpn added inline comments.
include/llvm/IR/IRBuilder.h
1238

I made one function that takes both. Patch coming in a minute.

kpn updated this revision to Diff 207829.Jul 3 2019, 10:32 AM

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 constrained intrinsics the fast math flags and the FP metadata is now handled exactly the same as in non-constrained mode. I've updated the documentation to warn about that when turning constrained mode on and off.

I was also a little worried about having CreateConstrainedFPBinOp() handle fast math flags differently from CreateBinOp(). Now they're the same.

Thanks, this looks good to me.

wuzish added a subscriber: wuzish.Jul 3 2019, 7:34 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 8 2019, 9:20 AM
Closed by commit rL365339: Teach the IRBuilder about fadd and friends. (authored by kpn, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 9:20 AM
This comment was removed by mibintc.