This is an archive of the discontinued LLVM Phabricator instance.

Add constrained intrinsics for lrint and lround
ClosedPublic

Authored by kpn on Jul 15 2019, 7:57 AM.

Diff Detail

Event Timeline

kpn created this revision.Jul 15 2019, 7:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 7:57 AM
kpn added a subscriber: ajwock.Jul 15 2019, 7:57 AM
docs/LangRef.rst
15705

I know you're just copying what we previously said about the constrained rint, but we should probably say that it "will" raise this exception.

15724

We should describe what is returned if the value is too large to be represented as a long. The llvm.lrint doesn't do that either, but it should too.

15736

It seems like we have a problem here. The intrinsics are overloaded to take any integer as a return type, but not all integers will match up with a real library call. Is that handled anywhere?

16010

Can you describe the rounding mode that will be used? You should also describe the conditions under which an exception will be raised.

include/llvm/CodeGen/ISDOpcodes.h
303

What's the logic behind the ordering here? If there's no good reason to insert new nodes in the middle of the existing list, it would be kinder to people maintaining out-of-tree branches if you would append them to the end of this group.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
208

Why are you changing this assertion?

2045

It's not clear to me why this is necessary now but hasn't been before. Since we're expanding to a library call, I don't think we need to preserve the chain that the strict FP node was using.

2062

If these changes do need to stay, you'll need to update this comment.

2940

Shouldn't something be getting pushed to Results here?

kpn marked 5 inline comments as done.Jul 16 2019, 7:34 AM
kpn added inline comments.
include/llvm/CodeGen/ISDOpcodes.h
303

Eh, I was just lumping the rounding nodes together. It's not important. Avoiding pain downstream overwhelms that weak reason. I'll change it.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
208

Because it is stronger than SelectionDAG::ReplaceAllUsesWith(SDNode *From, SDNode *To) needs it to be (unless I'm wrong), and because without this change the test case cannot pass.

The conversion to a libcall results in a node with result value, a chain, and glue. There was no glue before, so the added result would trigger the assert.

2045

So we don't need the chain to preserve ordering since it's a function call? That makes things simpler. Here, at least. I don't know what happens in ReplaceNode when a Chain result gets swapped out with a Glue result.

2062

I'd love to see a test case for this. You'll notice the assert() I added below because I don't have one. That would make it much easier to update the comment. it does need to be updated.

2940

No. If we go that route then we end up in ReplaceNode(SDNode *Old, const SDValue *New) with multiple SDValues to replace but only one of them being given. This results in ReplaceNode() running off into some other memory and typically crashing. We need to instead call ReplaceNode(SDNode *Old, SDNode *New) so all values that need to be looked at are present.

That's why I call ReplaceNode() myself here, and since we're bypassing the rest of the function I mostly-duplicated the debug message and the return statement.

kpn added inline comments.Jul 16 2019, 11:17 AM
docs/LangRef.rst
15724

What do we want this to be? My draft copy of C99 says the return value is "unspecified". What does that translate to in LLVM-land? Is this listed in IEEE 754?

docs/LangRef.rst
15724

That should throw an Invalid exception. And I think we agreed in your other Diff that it should return a poison value.

docs/LangRef.rst
15724
7.2 Invalid operation 

<...snip...>

For operations producing no result in floating-point format, the operations that signal the invalid operation exception are:

i) conversion of a floating-point number to an integer format, when the source is NaN, infinity, or a value that would convert to an integer outside the range of the result format under the applicable rounding attribute
kpn added inline comments.Jul 17 2019, 6:07 AM
docs/LangRef.rst
15724

Poison only in the constant folding case, right? What about in the non-constant-folding case?

cameron.mcinally added inline comments.
docs/LangRef.rst
15724

Poison only in the constant folding case, right? What about in the non-constant-folding case?

@eli.friedman

Besides the obvious flag raising, that would be undefined. It would be up to the hardware.

craig.topper added inline comments.Jul 17 2019, 9:48 PM
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
118

Why do these need to be handled here, but the none strict versions aren't?

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
174

We don't handle the non-vector version of these here. So I don't think we need to handle these yet.

lib/IR/Verifier.cpp
4250

Probably need to ensure these don't get used with vectors to match their none constrainted counterparts

efriedma added inline comments.
docs/LangRef.rst
15724

LangRef defines the semantics of the IR; what the current version of the optimizer does or does not constant-fold, or otherwise optimize, is irrelevant.

llvm.experimental.constrained.llrint should probably do the same thing as llvm.llrint, whatever that is, if the rounding mode is the default. If that somehow didn't get defined in LangRef, please make a separate patch for that.

15736

If you try to call an intrinsic where there is no underlying library call, it'll fail to compile, I assume. This doesn't really seem like a problem.

kpn updated this revision to Diff 214389.Aug 9 2019, 9:24 AM
kpn marked 5 inline comments as done.

Address review comments.

I found that a minor tweak in ExpandArgFPLibCall() to mutate strict nodes makes the changes elsewhere much nicer and similar to the other library calls. I then moved the conversion to libcalls for lrint() and lround() into the ConvertNodeToLibCall() function. This patch should now be more in line with the traditional handling of library calls.

kpn updated this revision to Diff 215932.Aug 19 2019, 9:37 AM

Tweak documentation hopefully as requested. Remove a stray blank line that somehow got in there.

craig.topper added inline comments.Aug 27 2019, 8:23 AM
lib/CodeGen/TargetLoweringBase.cpp
756

What places check the result? Can they be fixed?

kpn added inline comments.Aug 28 2019, 9:44 AM
lib/CodeGen/TargetLoweringBase.cpp
756

Hmmm, in LegalizeDAG.cpp we're checking the value in LegalizeOp around line 1153. We're checking the value in ExpandNode around 3692.

If these aren't libcalls then we'll do the mutation in SelectionDAGISel.cpp, but that checks the value as well. And it comes too late for mutating if it will become a libcall. That's what i'm seeing: a failure during instruction selection because mutation to LRINT happened too late.

I'm not sure, though, that they are broken. Operation actions don't seem to be defined exclusively for operands or exclusively for values. And there's oodles of code checking operands, but oodles of code checking values. Some by definition, since, for example, the Expand code must be looking at values.

Anything that isn't marked will default to Legal and cause us problems. I'm not sure we want to be playing whack-a-mole forever adding exceptions here and there. This is a bit of a shotgun approach, but it does cover all the bases we need covered.

I'll experiment a bit more and see what if I can get this thing to behave without this block of code here.

kpn updated this revision to Diff 217923.Aug 29 2019, 10:02 AM

Address review comments.

If I tweak ExpandNode() to know that we shouldn't attempt to bypass creating a libcall when encountering a strict lrint/lround then I no longer need to register these nodes for any of the integer types.

This looks good to me, but I'd like @craig.topper to verfiy that he's happy with the parts he commented on.

craig.topper accepted this revision.Oct 3 2019, 1:49 PM

LGTM other than that one comment.

lib/IR/Verifier.cpp
4724

Should this break be inside the curly braces? I don't think I've seen the style used here anywhere else.

This revision is now accepted and ready to land.Oct 3 2019, 1:49 PM
kpn closed this revision.Oct 7 2019, 9:34 AM

Changes pushed to r373900. I don't know why the ticket was left open.

nlopes added a subscriber: nlopes.Feb 15 2022, 10:43 AM

Hi,

The semantics described by patch for lrint regarding the rounding mode is a bit peculiar and different from all the other constrained FP instrinsics.
It says:

The rounding mode is described, not determined, by the rounding mode argument. The actual rounding mode is determined by the runtime floating-point environment. The rounding mode argument is only intended as information to the compiler.

So does this means that the only accepted rounding mode is !dynamic? Why is lrint different from the remaining intrinsics?

I was trying to implement the semantics of lrint in Alive2 and came across this discrepancy.

Thanks!

Hi,

The semantics described by patch for lrint regarding the rounding mode is a bit peculiar and different from all the other constrained FP instrinsics.
It says:

The rounding mode is described, not determined, by the rounding mode argument. The actual rounding mode is determined by the runtime floating-point environment. The rounding mode argument is only intended as information to the compiler.

So does this means that the only accepted rounding mode is !dynamic? Why is lrint different from the remaining intrinsics?

I was trying to implement the semantics of lrint in Alive2 and came across this discrepancy.

Thanks!

I believe the quoted statement above is both correct and also identical to the semantics of the rounding mode argument of all other constrained intrinsics. For all of them, the rounding mode argument is a promise by the user to the compiler, not an instruction to the compiler to change anything about the rounding mode. All of these operations will actually use the current default rounding mode, but the presence of a constrained rounding mode argument is an implicit promise by the user to the compiler that the current rounding mode has been set up in a particular way, and the compiler (e.g. for optimization purposes) may rely on that promise.

Hi,

The semantics described by patch for lrint regarding the rounding mode is a bit peculiar and different from all the other constrained FP instrinsics.
It says:

The rounding mode is described, not determined, by the rounding mode argument. The actual rounding mode is determined by the runtime floating-point environment. The rounding mode argument is only intended as information to the compiler.

So does this means that the only accepted rounding mode is !dynamic? Why is lrint different from the remaining intrinsics?

I was trying to implement the semantics of lrint in Alive2 and came across this discrepancy.

Thanks!

I believe the quoted statement above is both correct and also identical to the semantics of the rounding mode argument of all other constrained intrinsics. For all of them, the rounding mode argument is a promise by the user to the compiler, not an instruction to the compiler to change anything about the rounding mode. All of these operations will actually use the current default rounding mode, but the presence of a constrained rounding mode argument is an implicit promise by the user to the compiler that the current rounding mode has been set up in a particular way, and the compiler (e.g. for optimization purposes) may rely on that promise.

Thank you.
Can I read what you wrote as "if the rounding mode argument is not !dynamic and if it differs from the run-time rounding mode, the intrinsic returns poison"?

Essentially you want to allow the compiler to use the given rounding mode for optimizations, but still be able to lower the intrinsic to a single libcall that will use the run-time rounding mode, not the one given as argument.

I believe the quoted statement above is both correct and also identical to the semantics of the rounding mode argument of all other constrained intrinsics. For all of them, the rounding mode argument is a promise by the user to the compiler, not an instruction to the compiler to change anything about the rounding mode. All of these operations will actually use the current default rounding mode, but the presence of a constrained rounding mode argument is an implicit promise by the user to the compiler that the current rounding mode has been set up in a particular way, and the compiler (e.g. for optimization purposes) may rely on that promise.

Thank you.
Can I read what you wrote as "if the rounding mode argument is not !dynamic and if it differs from the run-time rounding mode, the intrinsic returns poison"?

Essentially you want to allow the compiler to use the given rounding mode for optimizations, but still be able to lower the intrinsic to a single libcall that will use the run-time rounding mode, not the one given as argument.

This matches my understanding. (Though I must admit I'm not 100% confident I fully understand the precise distinction between "undefined behavior", a poison value, and an undef value ... It's certainly one of those :-) But from my reading of the IR reference, "poison" does indeed look correct here.)

I believe the quoted statement above is both correct and also identical to the semantics of the rounding mode argument of all other constrained intrinsics. For all of them, the rounding mode argument is a promise by the user to the compiler, not an instruction to the compiler to change anything about the rounding mode. All of these operations will actually use the current default rounding mode, but the presence of a constrained rounding mode argument is an implicit promise by the user to the compiler that the current rounding mode has been set up in a particular way, and the compiler (e.g. for optimization purposes) may rely on that promise.

Thank you.
Can I read what you wrote as "if the rounding mode argument is not !dynamic and if it differs from the run-time rounding mode, the intrinsic returns poison"?

Essentially you want to allow the compiler to use the given rounding mode for optimizations, but still be able to lower the intrinsic to a single libcall that will use the run-time rounding mode, not the one given as argument.

This matches my understanding. (Though I must admit I'm not 100% confident I fully understand the precise distinction between "undefined behavior", a poison value, and an undef value ... It's certainly one of those :-) But from my reading of the IR reference, "poison" does indeed look correct here.)

OK, thank you!
It's poison. UB would be too strong. I've put on my todo list to fix LangRef as well.

This matches my understanding. (Though I must admit I'm not 100% confident I fully understand the precise distinction between "undefined behavior", a poison value, and an undef value ... It's certainly one of those :-) But from my reading of the IR reference, "poison" does indeed look correct here.)

OK, thank you!
It's poison. UB would be too strong. I've put on my todo list to fix LangRef as well.

The reason why I was hesitating about poison vs. UB is exceptions. This is clearer when talking about the exception metadata as opposed to the rounding mode metadata: if exception metadata promises that FP exceptions are disabled, but they are actually enabled at runtime, compiler optimizations are free to generate code triggering spurious exceptions that might kill the program outright - that seems more like UB than poison to me.

With the rounding mode, this is much less obvious to me, but I guess in theory it could happen that if the actual rounding mode does not match the mode promised by the metadata, a compiler optimization might possibly generate code that could now e.g. introduce an underflow where the unoptimized code would not have one, and if in addition exception are also enabled, that extra underflow could now trigger an extra exception. However, I'm not sure if that can actually ever occur in practice ...

This matches my understanding. (Though I must admit I'm not 100% confident I fully understand the precise distinction between "undefined behavior", a poison value, and an undef value ... It's certainly one of those :-) But from my reading of the IR reference, "poison" does indeed look correct here.)

OK, thank you!
It's poison. UB would be too strong. I've put on my todo list to fix LangRef as well.

The reason why I was hesitating about poison vs. UB is exceptions. This is clearer when talking about the exception metadata as opposed to the rounding mode metadata: if exception metadata promises that FP exceptions are disabled, but they are actually enabled at runtime, compiler optimizations are free to generate code triggering spurious exceptions that might kill the program outright - that seems more like UB than poison to me.

With the rounding mode, this is much less obvious to me, but I guess in theory it could happen that if the actual rounding mode does not match the mode promised by the metadata, a compiler optimization might possibly generate code that could now e.g. introduce an underflow where the unoptimized code would not have one, and if in addition exception are also enabled, that extra underflow could now trigger an extra exception. However, I'm not sure if that can actually ever occur in practice ...

Thank you!
Makes sense. I need to think a bit more about this.
Ideally we want to allow intrinsics marked as not throwing exceptions to be executed speculatively. The semantics needs to be crafted to allow that scenario.
I think returning poison for the rounding mode is OK as that is sufficient to trigger an exception (as poison can be replaced with any value). So we can keep the mismatch in rounding modes as poison. And then move the complexity to the exception semantics. It's not obvious to me what's the ideal semantics yet.

kpn added a comment.Feb 16 2022, 5:17 AM

This matches my understanding. (Though I must admit I'm not 100% confident I fully understand the precise distinction between "undefined behavior", a poison value, and an undef value ... It's certainly one of those :-) But from my reading of the IR reference, "poison" does indeed look correct here.)

OK, thank you!
It's poison. UB would be too strong. I've put on my todo list to fix LangRef as well.

The reason why I was hesitating about poison vs. UB is exceptions. This is clearer when talking about the exception metadata as opposed to the rounding mode metadata: if exception metadata promises that FP exceptions are disabled, but they are actually enabled at runtime, compiler optimizations are free to generate code triggering spurious exceptions that might kill the program outright - that seems more like UB than poison to me.

With the rounding mode, this is much less obvious to me, but I guess in theory it could happen that if the actual rounding mode does not match the mode promised by the metadata, a compiler optimization might possibly generate code that could now e.g. introduce an underflow where the unoptimized code would not have one, and if in addition exception are also enabled, that extra underflow could now trigger an extra exception. However, I'm not sure if that can actually ever occur in practice ...

Thank you!
Makes sense. I need to think a bit more about this.
Ideally we want to allow intrinsics marked as not throwing exceptions to be executed speculatively. The semantics needs to be crafted to allow that scenario.
I think returning poison for the rounding mode is OK as that is sufficient to trigger an exception (as poison can be replaced with any value). So we can keep the mismatch in rounding modes as poison. And then move the complexity to the exception semantics. It's not obvious to me what's the ideal semantics yet.

I'm a little worried about the runtime rounding mode being changed and speculative execution moving an instruction to a point where the wrong rounding mode is in effect. I think a dynamic rounding mode intrinsic call shouldn't be moved past a function call. I'm pretty sure the PowerPC backend already has this rule. But it's still possible for a function call to change the rounding mode with the rounding mode metadata properly representing this, but code movement optimizations might not take this into account and we'll have a problem.

I'm pretty sure the rounding metadata is there because the compiler can't know the actual rounding mode used at runtime. So I don't know how the compiler can know about UB and I don't know how it can use undef or poison.