Earlier in the year intrinsics for lrint, llrint, lround, and llround were added to llvm. The constrained versions are now implemented here.
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.
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.
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?
Can you describe the rounding mode that will be used? You should also describe the conditions under which an exception will be raised.
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.
Why are you changing this assertion?
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.
If these changes do need to stay, you'll need to update this comment.
Shouldn't something be getting pushed to Results here?
Eh, I was just lumping the rounding nodes together. It's not important. Avoiding pain downstream overwhelms that weak reason. I'll change it.
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.
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.
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.
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.
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
|118 ↗||(On Diff #209864)|
Why do these need to be handled here, but the none strict versions aren't?
|174 ↗||(On Diff #209864)|
We don't handle the non-vector version of these here. So I don't think we need to handle these yet.
Probably need to ensure these don't get used with vectors to match their none constrainted counterparts
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.
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.
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.