Page MenuHomePhabricator

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
15852

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.

15871

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.

15883

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?

16175

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?

2020

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.

2033

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

2930

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.

2020

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.

2033

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.

2930

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
15871

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
15871

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

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

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

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
4299

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

efriedma added inline comments.
docs/LangRef.rst
15871

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.

15883

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
784

What places check the result? Can they be fixed?

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

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.Thu, Oct 3, 1:49 PM

LGTM other than that one comment.

lib/IR/Verifier.cpp
4771

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.Thu, Oct 3, 1:49 PM
kpn closed this revision.Mon, Oct 7, 9:34 AM

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