This is an archive of the discontinued LLVM Phabricator instance.

Add constrained int->FP intrinsics
ClosedPublic

Authored by kpn on Oct 21 2019, 12:07 PM.

Details

Summary

This adds constrained intrinsics for the signed and unsigned conversions of integers to floating-point.

Diff Detail

Event Timeline

kpn created this revision.Oct 21 2019, 12:07 PM
kpn updated this revision to Diff 225936.Oct 21 2019, 12:11 PM

Full context this time.

kpn planned changes to this revision.Oct 23 2019, 9:40 AM

Lacks documentation. Sorry. I'm working on that now.

simoll added a subscriber: simoll.Oct 24 2019, 1:53 PM
kpn updated this revision to Diff 226490.Oct 25 2019, 1:04 PM

Added missing documentation. Added a case I missed earlier.

This probably needs tests that will lower to single instructions. E.g. llvm.experimental.constrained.uitofp.v4f32.v4i32 should lower to a cvtps2dq on SSE2.

Maybe testing an AVX512VL target would be interesting too. They have really good support for different CVT variants.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1020

Should these be clustered with STRICT_LRINT/etc?

2425

Nit-picky: does this preserve the rounding results and flags raised?

If the target doesn't support the itofp instruction, I'm not sure if we can do better anyway. Just wondering if anyone had thought this through...

2944

Is the early return necessary here? It stands out from the surrounding code...

kpn marked 3 inline comments as done.Oct 29 2019, 7:28 AM

This probably needs tests that will lower to single instructions. E.g. llvm.experimental.constrained.uitofp.v4f32.v4i32 should lower to a cvtps2dq on SSE2.

Maybe testing an AVX512VL target would be interesting too. They have really good support for different CVT variants.

For a first cut I was hoping to avoid dealing with Custom lowerings. That's why there's no v4* test cases. Is this something that can be handled when the X86 backend support gets to this point?

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1020

The regular U/SINT_TO_FP nodes are handled a couple of lines above. That makes these clustered close to the matching non-constrained versions, which seemed appropriate.

Is there a better place?

2425

Well, we don't have the metadata arguments at this point, so they can't be carried through to the extend/round instruction.

And I don't know how a target that doesn't have an itofp instruction would handle a round instruction. Maybe? I don't know.

2944

Yes, it's necessary. The normal path falls through to ReplaceNode() which can't handle, for example, replacing a node with two results with one that has only one. That's why we have to explicitly return the chain and then handle node replacement "by hand" here. You'll see this in other STRICT_* cases in this switch.

In D69275#1725323, @kpn wrote:

This probably needs tests that will lower to single instructions. E.g. llvm.experimental.constrained.uitofp.v4f32.v4i32 should lower to a cvtps2dq on SSE2.

Maybe testing an AVX512VL target would be interesting too. They have really good support for different CVT variants.

For a first cut I was hoping to avoid dealing with Custom lowerings. That's why there's no v4* test cases. Is this something that can be handled when the X86 backend support gets to this point?

Ah, right. I forgot about that. That's reasonable.

Besides the one inline comment, this all looks good. It's a big patch though, so I'll wait before accepting to give others time for review.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1020

I think the STRICT_LRINT/etc cases should be moved up and combined with these new cases. All the non-strict variants are clustered together. It would make sense to cluster the strict variants directly under those.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1020

I suppose STRICT_LRINT/etc could be moved in a separate NFCI patch and then this patch just rebased. That's probably best. Sorry, just thinking aloud...

Do we need a Verifier check to ensure that the vector element counts match between source and dest type? Not sure what we do for the other direction.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2355

Ternary on the operand number?

2488

Drop the else since we early returned?

2945

Drop the else?

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
1110

Is this longer than 80 columns?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6035

A ternary operator on the operand number would probably be shorter?

llvm/test/CodeGen/X86/fp-intrinsics.ll
423

This check line is for IR, but this is an assembly test. CHECK isn't a valid check-prefix for this file. Which also means all of the CHECK-LABEL lines are broken in the existing tests aren't doing anything

kpn marked 7 inline comments as done.Oct 30 2019, 7:18 AM
kpn added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1020

When one does something like this one notices that I'm calling TLI.getOperationAction() instead of TLI.getStrictFPOperationAction(). That seems like a bug in one of the two places. Hmmm.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
1110

Yes. I'll run the patch through clang-format before my next upload.

llvm/test/CodeGen/X86/fp-intrinsics.ll
423

Ouch, yeah, that error is all over this file. How about I fix it for the new changes here and then fix the rest in another ticket?

kpn updated this revision to Diff 227142.Oct 30 2019, 10:49 AM

Address review comments.

craig.topper added inline comments.Oct 30 2019, 11:00 AM
llvm/test/CodeGen/X86/fp-intrinsics.ll
423

What is the expected behavior here? We seem to doing some emulation of the operation instead of using the conversion instruction we have with SSE2.

kpn marked an inline comment as done.Oct 30 2019, 11:21 AM
kpn added inline comments.
llvm/test/CodeGen/X86/fp-intrinsics.ll
423

That may be an artifact of of the getOperationAction()/getStrictFPOperationAction() issue that I found after the rearranging Cameron asked for. I'm looking into it now.

kpn planned changes to this revision.Oct 31 2019, 10:09 AM
kpn marked an inline comment as done.

It looks like I am going to have to do some Custom lowering work after all. Plus the changes to the IR verifier are missing.

llvm/test/CodeGen/X86/fp-intrinsics.ll
423

Yep, that's what caused it. If we use the getStrictFPOperationAction() route like the other STRICT nodes here then we end up with Custom lowerings.

It's a shame there's no "Unassigned" "lowering". Currently there's no way to know if the target doesn't implement something or if it really does want to Expand like is currently returned. And LegalizeOp() doesn't bother trying to query the target like ExpandNode() does. That seems like a bug.

In D69275#1728938, @kpn wrote:

It looks like I am going to have to do some Custom lowering work after all. Plus the changes to the IR verifier are missing

llvm/test/CodeGen/X86/fp-intrinsics.ll
423

I didn't understand this. Isn't ExpandNode is only called from LegalizeOp? And doesn't LegalizeOp query the target to know to call ExpandNode?

kpn marked an inline comment as done.Oct 31 2019, 11:48 AM
kpn added inline comments.
llvm/test/CodeGen/X86/fp-intrinsics.ll
423

Both true, yes. But for STRICT nodes LegalizeOp calls getStrictFPOperationAction(), which just calls getOperationAction() for the non-strict version of the node. So there's no way for LegalizeOp to know if the target wants to handle the STRICT node any differently from the non-strict. It never actually calls getOperationAction() for a STRICT node.

uweigand added inline comments.Nov 4 2019, 9:16 AM
llvm/test/CodeGen/X86/fp-intrinsics.ll
423

Maybe I'm confused here, but I thought I fixed this with my recent changes.

These days, all code should always call getOperationAction on the strict node first, and only if that returns Expand, then it should call getStrictFPOperationAction to find out how the expansion is to be implemented.

That means in LegalizeOp you should not call getStrictFPOperationAction. Really only ExpandNode should do so ...

pengfei added a subscriber: pengfei.Nov 4 2019, 9:22 PM
kpn updated this revision to Diff 228714.Nov 11 2019, 9:28 AM
kpn edited the summary of this revision. (Show Details)

Address review comments. Add missing IR verifier pieces. Add the remaining test cases.

I've added the bare minimum X86 support to get the tests running.

craig.topper added inline comments.Nov 11 2019, 10:56 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1248

Should we just be returning MERGE_VALUES from the custom handler for the strict fp nodes? That's how we handle loads and atomics.

kpn marked an inline comment as done.Nov 11 2019, 11:42 AM
kpn added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1248

I'll take a look. Making this code simpler would be nice.

llvm/docs/LangRef.rst
15589

This sounds incomplete. I think some of the information from the arguments section ought to be here instead.

llvm/test/CodeGen/X86/fp-intrinsics.ll
481

I feel like this should also be checking for the mov instruction, which implicitly zero-extends the 32-bit input. Maybe even a comment explaining what happened, because just looking at the test and seeing that uitofp gets lowered to cvtsi2sd looks wrong.

499

This sequence is quite opaque. I'm not sure it's the best way of handling this case, though I realize that's unrelated to your patch. What is related to your patch is the question of whether the subpd here might be introducing a spurious exception. I'd need to figure out what this is doing to answer that question.

530

Some more context would be useful here. I believe there is a test a jump and an addss that are also relevant in the output.

I've switched the fp-intrinsic.ll test to use the update_llc_test_checks.py script and added more command lines in commit 774e829c29017d35e8af3b854f21c792caf30181. Ialso added more RUN lines to vector-constrained-fp-intrinsics.ll in 9e5116f756f05b68e8394e392027dca7bc574559 Can you rebase the tests here with those changes?

kpn marked an inline comment as done.Nov 12 2019, 7:32 AM

I like the switch to using update_llc_test_checks.py. Sure, I'll rebase, run that script and it'll be in the next round of this patch when I can.

llvm/test/CodeGen/X86/fp-intrinsics.ll
530

These issues are solved by switching to using the update_llc_test_checks.py script that Craig ran. I'll have it in my next round of this patch when I can get to it.

Some comments on verifying the various algorithms for correctly respecting constrained FP semantics. I have not looked at the X86 back-end.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
1020

Yes, the code below for STRICT_LRINT etc seems buggy, it should definitely call TLI.getOperationAction instead.

2425

I don't believe we need the metadata arguments here. Instead, we need to verify that this code sequence always rounds correctly according to the current rounding mode, no matter what it is, and also that this code sequence raises the same set of exceptions that a real IEEE compliant itofp operation would raise.

So looking at the algorithm, it first constructs two double values "by hand", using only integer operations, which is always fine. Then those two are subtracted from one other. Due to the construction, neither can be NaN, and also the difference is always exactly representable. This means that the FSUB never raises any exception and does not depend on the current rounding mode, so this should also be fine. (So possibly, we don't even need a STRICT_FSUB here but can continue using a normal FSUB?)

Finally, the result may be extended or rounded to another floating-point type. Extension should be fine since it never rounds and the input cannot be a NaN. Rounding should also be fine since it uses the correct (current) rounding mode, due to the construction of the input can never raise underflow or overflow exceptions, and raises the inexact exception exactly when it ought to be raised.

2448

This is actually the more problematic case, because this may round the wrong way depending on the current rounding mode.

For example, if the input is a large i64 value that does not exactly fit into the result type, and the rounding mode is "towards zero", then the result should be the FP value immediately smaller than the exact result. But since we're actually doing a signed conversion, the input will be interpreted as a negative value (of large absolute value), and rounded "towards zero", i.e. to the FP value immediately larger than the exact result of converting the negative value. After adding back the bias, we'll then have the FP value immediately larger than the exact result of an (unsigned) conversion of the original value, which is incorrect.

It actually looks like this this has nothing to do with constrained FP semantics, this code simply gives the wrong result even for regular FP operations ...

(GCC avoid this by either converting to a larger intermediate FP type first, if possible, or else avoiding the rounding issue by ensuring only positive values are passed to the signed-to-FP intermediate conversion.)

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1272

This algorithm however looks fine to me. The SINT_TO_FP operations always operate on positive values, and even more so, since they only use a half-word, the result is guaranteed to fit exactly into the target FP type, so there is never any rounding or exception. (These then don't really need to be strict operations.) The FMUL only increments the exponent, so again there is no rounding or exception.

The FADD at the end rounds correctly, and raises the correct exceptions, so this looks all good.

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2653

This confuses me again. It seems this may generate a SINT_TO_FP -> HalfVT -> FP_ROUND -> OutVT chain, which introduces a potential double rounding that can lead to incorrect results even disregarding any constrained FP semantics ...

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6076

This also looks correct to me. The STRICT_SINT_TO_FP will round correctly in any rounding mode, if my understanding of the algorithm is correct, and it will also raise the inexact exception if appropriate. The STRICT_FADD is just a multiply by two, which does not depend on rounding and cannot raise any exceptions given the input (so it might as well be just a plain FADD).

But what confuses me a bit again is why we have two algorithms for UINT_TO_FP expansion: a correct one here, and the incorrect one above in ExpandLegalINT_TO_FP? Under what circumstances will we ever end up using the incorrect one?

6129

Same comment as above.

craig.topper added inline comments.Nov 26 2019, 12:23 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2959

Extra indentation

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
1106

I think the input chain should be an argument here if we're going to make this a reusable function. Op could theoretically be a node with 2 data results and a chain which would make this wrong. It should probably also return std::pair of the two results so the caller doesn't have to assume where the chain is for nodes that were created inside.

craig.topper added inline comments.Nov 26 2019, 3:15 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2653

For the only case we have tests for i64->f16. I think any integer value large enough to cause rounding when converted to f32 would be too large to represent at all in f16. Since f16's max exponent of 15 is less than the length of f32's mantissa.

uweigand added inline comments.Nov 27 2019, 7:02 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2653

Ah yes, you're right. So this should be fine with non-strict semantics.

And for strict semantics, we should also be fine. The int->f32 conversion can only raise an inexact exception, and this only in cases where the int->f16 conversion should raise an inexact. The f32->f16 conversion due to construction of the input also can only raise an inexact exception, and again only in cases where we should have one. Conversely, in every case where we should have an inexact exception, one (or both) of the intermediate steps will raise it. (I think there may be cases where we get two, but that should be fine even for strict semantics.)

uweigand added inline comments.Nov 27 2019, 7:58 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2653

Following up on myself: of course the f32->f16 conversion can also overflow, but again only in cases (and in fact in exactly in those cases) where the original int->f16 conversion should have overflowed. So again this should be fine.

Sorry again for the confusion, I'm not really used to thinking about f16 :-)

craig.topper added inline comments.Nov 27 2019, 8:08 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2653

We should probably fix the code only do this for f16. I think we could construct a vXi128->vXf32 test that would go through this and I think it would be wrong.

craig.topper added inline comments.Nov 27 2019, 9:25 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2653

Nevermind, I think we don't use the algorithm if the input type will be scalarized. I expect vXi128 will be scalarized on all targets.

kpn updated this revision to Diff 231300.Nov 27 2019, 11:05 AM

Update for review comments. Use MERGE_VALUES node. Eliminate hacks to avoid x86 custom lowering. This means more x86 target support.

I have not addressed the algorithm issues pointed out by Ulrich. So far I'm trying to just get the existing algorithms converted for strict use. Would it make sense to hold off on algorithm changes until another patch?

craig.topper added inline comments.Nov 27 2019, 11:32 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
18764

Use Sub.getValue(1)

19011

You can just use Fild.getValue(1)

19036

Use getValue

19044

Use getValue

craig.topper added inline comments.Nov 27 2019, 11:32 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
988

Line the arguments up with the l ine above. Same for the two below

18521

Please add a FIXME

18696

getValue

18697

getValue

18717

You can DAG.getMergeValues which takes less arguments

18766

Don't we need to propagate the chain result too?

18783

Pleas add a FIXME here

kpn marked an inline comment as done.Nov 27 2019, 12:04 PM
kpn added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
18766

When given a strict FP node LowerOperation() will require that the resulting new node has the chain in the same place in the values. So, no, I don't think so unless we want to thread the std::pair all the way back up.

craig.topper added inline comments.Nov 27 2019, 12:35 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
18766

Oh this works because the node is either a STRICT_FP_EXTEND or STRICT_FP_ROUND so the node in first and second is the same. But we should probably use a MERGE_VALUES here to not rely on that.

craig.topper added inline comments.Nov 27 2019, 1:26 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7349

This only works if N2 is result 1 of N1.

In D69275#1762098, @kpn wrote:

I have not addressed the algorithm issues pointed out by Ulrich. So far I'm trying to just get the existing algorithms converted for strict use. Would it make sense to hold off on algorithm changes until another patch?

For me this would be fine as a first step, but we should at least add FIXME statements to those algorithms where we already know there are strict semantics problems.

uweigand added inline comments.Nov 29 2019, 7:36 AM
llvm/include/llvm/CodeGen/TargetLowering.h
4125

Should document Chain here.

Or maybe it would be better to use MERGE_VALUES to return both components if appropriate?

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
176

If we pass Node, then all other arguments are really redundant, aren't they?

178

Hmm, wouldn't we also need to update this routine? Or can we say that promotion is not appropriate for handling strict semantics anyway?

2949

Why do we have to do the Replace... thing here instead of just appending both to Results (like is done for other nodes with chain)?

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
329

This should now also use ValVT, I think.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9156

Hmm. There's an UnrollVectorOp_StrictFP in LegalizeVectorTypes.cpp. If we change this routine to also handle some strict operations, maybe we should go all the way and just have merge the routines completely?

llvm/lib/IR/Verifier.cpp
4791

I believe the NumOperands check is now redunant with common tests handled above.

kpn marked an inline comment as done.Dec 2 2019, 6:34 AM
kpn added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7349

I don't understand. N2 is a constant that is either 0 or 1. What will happen if it is discarded here?

This code was lifted straight out of getNode() somewhere around line 5194. Without it the X86 target dies trying to lower a rounding of f64 to f64. This happens because getStrictFPExtendOrRound() returns a round when input and output are the same size. This mirrors the non-strict getFPExtendOrRound().

craig.topper added inline comments.Dec 2 2019, 9:12 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7349

Oops you’re right about N2. But there is an issue with the chain. Which I think is what I was thinking N2 was when I wrote that. If the input chain didn’t come from N1 this is broken.

I don’t see any precedent for returning a MERGE_VALUES from getNode. I think we need to fix the caller of getStrictFPExtendOrRound to only call when necessary.

craig.topper commandeered this revision.Dec 6 2019, 12:24 AM
craig.topper edited reviewers, added: kpn; removed: craig.topper.

Commandeering to update the X86 code and some other fixes/cleanup.

craig.topper marked an inline comment as done.

-Improve some of the X86 code.
-Add Promote support. Use it for i8/i16 on X86.
-Remove changes to UnrollVectorOp which seemed to be unexercised
-Some cleanup in ExpandLegalINT_TO_FP
-Drop the changes to getNode. We can't fold NOP conversions here and asserts were recently added in another patch.

Upload with context

Change the signature of SelectionDAGLegalize::ExpandLegalINT_TO_FP to allow it to update the Results vector directly.

kpn marked 6 inline comments as done.Dec 6 2019, 5:04 AM

I'm still working on this ticket daily! I'm trying to merge the two vector unrolling functions like Ulrich suggested. But I ran into problems that lead me to think we may have a serious issue lurking that we'll need to fix. That's what I've been working on: trying to understand the issue and see if it needs further investigation.

If you are in a hurry then you could have sent me an email and I would have uploaded the diffs I've got without further investigation.

I'm leaving attached the comments on my work that I've been adding but haven't submitted until now.

llvm/include/llvm/CodeGen/TargetLowering.h
4125

Documentation. Check. I'll have that in my next round.

I don't remember why the prototype for expandFP_TO_UINT() got reformatted. But this code is already in the tree. Having expandUINT_TO_FP() be consistent with the existing tree seems like a good idea?

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
176

Seems that way. I'll give it a try.

178

I don't have a test for it so I didn't change it.

Can we guarantee the result would be rounded back down? Seems like promotion would be invalid without that guarantee.

2949

I've had a lot of trouble with this, but at the moment I'm unable to reproduce any issue here. Let's simplify and see how it goes.

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
329

Looks like it.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7349

Done. I've also added an assert to document that getStrictFPExtendOrRound() wants the lengths to be different.

In D69275#1772541, @kpn wrote:

I'm still working on this ticket daily! I'm trying to merge the two vector unrolling functions like Ulrich suggested. But I ran into problems that lead me to think we may have a serious issue lurking that we'll need to fix. That's what I've been working on: trying to understand the issue and see if it needs further investigation.

If you are in a hurry then you could have sent me an email and I would have uploaded the diffs I've got without further investigation.

I'm leaving attached the comments on my work that I've been adding but haven't submitted until now.

Sorry. I should have sent a mail. If you want to upload over my last patch, I can merge my X86 changes into it.

What’s the serious issue you’ve encountered?

Restore @kpn 's last diff.

kpn added a comment.Dec 9 2019, 5:00 AM

The calls DAG.UnrollVectorOp() can't be used in place of DAGTypeLegalizer::ReplaceValueWith() because then the replaced node doesn't get properly deleted. This then results in an assertion failure later because the replaced node wasn't legal. And having SelectionDAG call into DAGTypeLegalizer doesn't seem like a winning idea, at least not without some plumbing. So today I'm going to punt on merging those functions together and just post what I've got. After breakfast sometime.

kpn commandeered this revision.Dec 10 2019, 6:04 AM
kpn edited reviewers, added: craig.topper; removed: kpn.

Prep to update patch.

kpn updated this revision to Diff 233072.Dec 10 2019, 6:06 AM

Update for review comments.

I've looked over the common code changes again, and they now look good to me. I'll leave it to Craig to review the X86 changes ...

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
178

Right. In general promotion is not appropriate for strict semantics because you don't get the right exceptions (for overflow etc.).

2403

NewChain seems superfluous?

kpn updated this revision to Diff 233144.Dec 10 2019, 10:40 AM

Eliminate write-only variable.

kpn marked an inline comment as done.Dec 10 2019, 10:41 AM
kpn added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2403

Right, fixed now. Thanks!

craig.topper added inline comments.Dec 10 2019, 10:41 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
178

Promotion is definitely bad for fp->int, but is there really an issue for int->fp? We're just going to use a bigger int for the input. If the small int was going to overflow, it should still overflow when its extended.

uweigand added inline comments.Dec 10 2019, 10:57 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
178

Ah, you're right. This should be fine.

@kpn: If you're looking for a test case, this would most likely involve something like a i16->f32 conversion.

craig.topper added inline comments.Dec 10 2019, 11:27 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1160

Can a strict node get here? The call site in vectorLegalizer::Expand only checks for the non-strict node.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9145

When are these changes needed? The STRICT_UINT_TO_FP handling in LegalizeVectorOps always scalarizes and goes through ExpandStrictFPOp.

craig.topper added inline comments.Dec 10 2019, 11:44 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2397

Can we merge the 2 strict fp ifs here? The only thing we do between them is declare a new variable.

2405

Why is this not just DestVT != Sub.getValueType()

llvm/lib/Target/X86/X86ISelLowering.cpp
18704

We probably do need a strict version of FHADD, but until we have that we should just go to the shuffle + STRICT_FADD code below rather than silently dropping the chain.

craig.topper added inline comments.Dec 10 2019, 12:48 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
18989

This looks out of date. I recently changed this to return a std::pair of Result and Chain so it was obvious that the chain result was intended as an output. So we need a merge values here for strict fp now. This should be reflected in https://reviews.llvm.org/D71130

kpn marked 6 inline comments as done.Dec 11 2019, 1:02 PM

Huh. I forgot to submit my comments earlier.

Anyway, I've merged in many changes from D71130, I've made a few changes as requested in LegalizeDAG.cpp, and I added the missing call to ExpandUINT_TO_FLOAT(). Now I'm seeing X86::VMULPDYrr die when running the vector-constrained-fp-intrinsics.ll test on the v4i32 type. It seems that in InstrEmitter.cpp it dies at the assertion at line 838 because NumOperands is 3. That's as far as I've gotten today.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
178

Agreed. I'm working on LegalizeDAG.cpp right now. I notice X86 already has Promote for i8 and i16.

2397

Done.

2405

Eh, I figured it'd be more clear this way. I'll change it.

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1160

Ah, I must have lost the change to call here, possibly with D69887 going in.

llvm/lib/Target/X86/X86ISelLowering.cpp
18704

Change lifted from D71130.

18989

Yup, I just needed to rebase.

kpn updated this revision to Diff 233440.Dec 11 2019, 1:04 PM

Address review comments, with changes to LegalizeDAG.cpp and LegalizeVectorOps.cpp.

One comment unaddressed still.

craig.topper added inline comments.Dec 11 2019, 3:36 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1209

Src here should be Op.getOperand(0) I think. I think that's causing your MULPD crash.

1362

Can this code be replaced with DAG.UnrollVectorOp now? Or should we use this code for strict instead of DAG.UnrollVectorOp for ExpandUINT_TO_FLOAT?

kpn marked an inline comment as done.Dec 12 2019, 5:53 AM
kpn added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1209

That was it. Thanks!

kpn marked an inline comment as done.Dec 13 2019, 11:23 AM
kpn added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9145

VectorLegalizer::ExpandUINT_TO_FLOAT() uses UnrollVectorOp() if the required instructions are not available. I have a change where it gets called from the top of ExpandStrictFPOp() and instead returns an empty value if it would have called UnrollVectorOp(). I think that may eliminate the need to modify UnrollVectorOp() since it will then fall through into the ExpandUINT_TO_FLOAT() logic instead. If it works it'll generate better code, and eliminating complexity would be nice.

kpn updated this revision to Diff 234104.Dec 16 2019, 10:42 AM

Address review comments. I've eliminated the need to call SelectionDAG::UnrollVectorOp() and therefore eliminated the changes to that function.

kpn marked an inline comment as done.Dec 16 2019, 10:46 AM
kpn added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1362

I addressed this by eliminating the need to call DAG.UnrollVectorOp(). That function is now out of the discussion. The tests show that this way here usually results in shorter sequences of instructions as well.

General question for you and @uweigand that I realized today. Do we need to set the FPExcept bit in the flags for new nodes when we expand/promote operations?

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1182

Isn't SDValue(nullptr, 0) equivalent to SDValue() and more consistent with other code?

1361

Can we just declare SDValue Res in the if condition so we don't need double parentheses?

kpn updated this revision to Diff 234122.Dec 16 2019, 12:53 PM

Address review comments.

This revision is now accepted and ready to land.Dec 16 2019, 12:56 PM

General question for you and @uweigand that I realized today. Do we need to set the FPExcept bit in the flags for new nodes when we expand/promote operations?

Ah yes, we need to do that.

This reminds me of a general concern: for all other flag bits, omitting the flag is conservatively safe, it just may impact performance. But for FPExcept, omitting the flag impacts correctness. Maybe we should go ahead and invert the sense of the flag (i.e. use a FPNoExcept flag instead of FPExcept) after all ...

This revision was automatically updated to reflect the committed changes.