Earlier in the year intrinsics for lrint, llrint, lround, and llround were added to llvm. The constrained versions are now implemented here.
andrew.w.kaylor craig.topper hfinkel mehdi_amini aemerson javed.absar uweigand kbarton cameron.mcinally
- rG1c3d19c82d93: [FPEnv] Add constrained intrinsics for lrint and lround
rGa6fc72fba9dc: Fix sphinx warnings.
rL373902: Fix sphinx warnings.
rL373909: Fix another sphinx warning.
rL373900: [FPEnv] Add constrained intrinsics for lrint and lround
rG9f4de84eb0e0: Fix another sphinx warning.
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.
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.
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.