Page MenuHomePhabricator

Add constrained intrinsics for lrint and lround
Needs ReviewPublic

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

Details

Summary

Earlier in the year intrinsics for lrint, llrint, lround, and llround were added to llvm. The constrained versions are now implemented here.

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
15757

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.

15776

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.

15788

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?

16078

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–304

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?

2011

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.

2024

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

2904

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–304

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.

2011

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.

2024

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.

2904

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
15776

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
15776

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
15776
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
15776

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

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

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 ↗(On Diff #209864)

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

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
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.

lib/IR/Verifier.cpp
4251

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

efriedma added inline comments.
docs/LangRef.rst
15776

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.

15788

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.Fri, Aug 9, 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.