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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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 ...
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 1244 ↗ | (On Diff #176432) | I think you can forgo the else { in these functions since the if branch returns immediately. |
Comment Actions
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?
Comment Actions
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 ↗ | (On Diff #176464) | This is a minor quibble, but the method is setIsConstrainedFP, while the member is IsFPConstrained. |
Comment Actions
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 ↗ | (On Diff #176464) | Yeah, that's an oversight. Fixed. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 234 ↗ | (On Diff #176464) | 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. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 110 ↗ | (On Diff #176464) | Instead of adding these here, make these inline initializers on lines 100-102. |
| 228 ↗ | (On Diff #176464) | IS this fixed? |
| 1249 ↗ | (On Diff #176464) | Why set the last 2 to nullptr when you have defaults for these? |
| 1259 ↗ | (On Diff #176464) | 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 ↗ | (On Diff #176464) | All these CreateConstrainedXXX should be distilled down to a single function that takes the intrinsic as a parameter. |
Comment Actions
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 ↗ | (On Diff #176464) | 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 |
Comment Actions
I don't know which patch will proceed; but they weren't added already, this seems to be missing documentation changes.
Comment Actions
I wanted to get the API straight before working on documentation.
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 228 ↗ | (On Diff #176464) | In my working copy, yes. |
| 234 ↗ | (On Diff #176464) | It's a good idea. D62730 beat me to the punch. |
| 1249 ↗ | (On Diff #176464) | Eh, no really good reason. |
| 1259 ↗ | (On Diff #176464) | 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 ↗ | (On Diff #176464) | 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. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 234 ↗ | (On Diff #176464) | 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? |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.)
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 113 ↗ | (On Diff #202987) | 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. |
| 255 ↗ | (On Diff #202987) | This seems unnecessary. |
| 1138 ↗ | (On Diff #202987) | 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. |
| 228 ↗ | (On Diff #176464) | 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. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 113 ↗ | (On Diff #202987) | 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? |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 113 ↗ | (On Diff #202987) | 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. |
Comment Actions
Address review comments.
Run the changes through clang-format.
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 1138 ↗ | (On Diff #202987) | 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. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 1138 ↗ | (On Diff #202987) | 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. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 1138 ↗ | (On Diff #202987) | Ok, will do. |
Comment Actions
Add static methods to convert between a StringRef and the enums for RoundingMode or ExceptionBehavior.
| 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. |
| 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>. |
Comment Actions
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.
| 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. |
| include/llvm/IR/IntrinsicInst.h | ||
|---|---|---|
| 235 ↗ | (On Diff #205371) |
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.
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. |
Comment Actions
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.
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 259 ↗ | (On Diff #207378) | 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. |
| 1324 ↗ | (On Diff #207378) | FPMD is dropped in this case; I don't know if that's intentional. |
| 1459 ↗ | (On Diff #207378) | It looks like nothing in IRBuilder ever passes these arguments. Are you just anticipating that someone might want to call this directly? |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 259 ↗ | (On Diff #207378) | 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. |
| 1324 ↗ | (On Diff #207378) | 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. |
| 1459 ↗ | (On Diff #207378) | Correct. And I wrote tests for it in IRBuilderTest.cpp. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 259 ↗ | (On Diff #207378) | 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. |
| 1324 ↗ | (On Diff #207378) | 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? |
| 1459 ↗ | (On Diff #207378) | Okay. So people who want to pass these explicitly just can't use the convenience methods? That seems like a reasonable compromise. |
| include/llvm/IR/IRBuilder.h | ||
|---|---|---|
| 1324 ↗ | (On Diff #207378) | I made one function that takes both. Patch coming in a minute. |
Comment Actions
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.
This comment was removed by mibintc.