Page MenuHomePhabricator

[FPEnv][X86] Constrained FCmp intrinsics enabling on X86
ClosedPublic

Authored by pengfei on Thu, Nov 21, 6:29 PM.

Details

Summary

This is a follow up of D69281, it enables the X86 backend support for the FP comparision.

Diff Detail

Event Timeline

pengfei created this revision.Thu, Nov 21, 6:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 21, 6:29 PM
pengfei updated this revision to Diff 230582.Thu, Nov 21, 7:35 PM

Fix a bug & Add Fix me to tests.

@pengfei : I've updated the D69281 patch, which will require follow-on changes here. In particular, signaling comparisons (STRICT_FSETCCS) need to be handled.

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

I guess this needs an input chain, right? I think you'll probably have to add a whole new STRICT_FSELECT_CC node to get this right ...

3665

Similarly here, although BR_CC of course already has a chain.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2795 ↗(On Diff #230582)

This is now already done in my base patch.

@pengfei : I've updated the D69281 patch, which will require follow-on changes here. In particular, signaling comparisons (STRICT_FSETCCS) need to be handled.

Thanks! I'm working on it now.

pengfei marked 3 inline comments as done.Thu, Dec 5, 6:37 PM
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3595

Since we are supporting both signaling and quite STRICT intrinsics, I think there's no need to expand the STRICT version of SETCC, SELECT_CC and BR_CC now.
That's because setCondCodeAction doesn't distinguish signaling and quite. Obviously, some CC that is legal in signaling or quite might be not in another.
The only way is customizing them, otherwise falling back to non STRICT node.

3665

Same as above.

craig.topper added inline comments.Thu, Dec 5, 7:02 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3595

Is it never legal to fall back to a non strict node. Once all targets support strict fp all mutation code will be removed from the tree. We might need to enhance setCondCodeAction to understand signaling.

pengfei marked an inline comment as done.Thu, Dec 5, 8:02 PM
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3595

We can enhance setCondCodeAction, but it's hard to enhance the expansion math here.
I did some research work on SSE and found we could happen expand to all 32 cases using the 8 legal CCs. But it is based on we know the behavior of each 8 CCs, so it can be hard coded. I think it's hard to give a general expansion in common code and may be easily result into infinite loop because we have to expand more than once for some CCs in SSE.
I agreed falling back to a non strict node is meanness. I think currently we have provided scalarization for vector operations' falling back. For targets support strict fp, they should at least support all cases in scalar operations either legal or custom.

pengfei marked an inline comment as done.Fri, Dec 6, 12:52 AM
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3595

I think you are correct! After some research, I believe we still need to enhance setCondCodeAction to at least get the action is legal or custom. And a minimal expansion like what non strict node doing is still useful. Thanks!

pengfei updated this revision to Diff 232850.Mon, Dec 9, 7:18 AM

Add signaling intrinsic handling.

craig.topper added inline comments.Mon, Dec 9, 3:31 PM
llvm/include/llvm/CodeGen/TargetLowering.h
179 ↗(On Diff #232850)

This enum name should mention compare or setcc or something its name. ActionIndex isn't going to read as being related to FP compares. Also, its not an Index, its a bit mask so using Index in the name is misleading

180 ↗(On Diff #232850)

Quite -> Quiet

2099 ↗(On Diff #232850)

Are we using the new support you've added here in this patch? If its not needed by X86 it doesn't belong in this patch.

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

Can we add to the Results vector here instead of doing manual replacement?

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

IsSignaling->IsSignalling

20851

singaling->signalling

20899

signaling->signalling

20911

Why is this change needed?

21298

Space before :

21302

Please run clang-format on this. I think the = should be at the end of the line above

21307

Is this something we need to fix? if so put a fixme here

21462

Signaling->Signalling

llvm/test/CodeGen/X86/fp80-strict-scalar-cmp.ll
4

Is the explicit sse disable on the 64-bit line necessary?

llvm/test/CodeGen/X86/vec-strict-128-cmp.ll
2

There's no return instructions in your test checks, so I don't think this was truely generated with the script.

10

Can you name the test functions after what condition code and and signalling vs quiet? Same for all the new test files

pengfei updated this revision to Diff 233005.Mon, Dec 9, 11:08 PM
pengfei marked 3 inline comments as done.

Address review comments.

pengfei marked 12 inline comments as done.Mon, Dec 9, 11:17 PM

Thanks for the review!

llvm/include/llvm/CodeGen/TargetLowering.h
2099 ↗(On Diff #232850)

Yes. We are using it for CCs SETOEQ and SETUNE in all scalar FP types.

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

I think so. Thanks!

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

They have the same meaning here. Changes reverted.

21307

Removed.
I was intending to enable fp128. But it wouldn't be enabled in this patch now.

llvm/test/CodeGen/X86/fp80-strict-scalar-cmp.ll
4

Removed, thanks!

craig.topper added inline comments.Mon, Dec 9, 11:38 PM
llvm/include/llvm/CodeGen/TargetLowering.h
2099 ↗(On Diff #232850)

But I couldn't find any modified calls to setCondCodeAction, so aren't we just always passing "Both"?

pengfei updated this revision to Diff 233007.Mon, Dec 9, 11:43 PM

Fix few format problems.

pengfei marked an inline comment as done.Tue, Dec 10, 12:00 AM
pengfei added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
2099 ↗(On Diff #232850)

Yes. We always use Both due to we have 2 identical instructions for quite and signalling scalar operations.
Do you mean we should keep a single table here? I think using 2 tables makes code clear, because we will get each of them in LegalizeSetCCCondCode with flag IsSignalling.

craig.topper added inline comments.Tue, Dec 10, 12:20 AM
llvm/include/llvm/CodeGen/TargetLowering.h
2099 ↗(On Diff #232850)

How does it make the code clearer? Just not passing IsSignalling to getCondCodeAction seems fine to me. I'm not saying I'm opposed to the change, but it doesn't help X86 so we need to find a target that benefits from it. Otherwise its just extra complexity.

craig.topper added inline comments.Tue, Dec 10, 12:37 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3557

Don’t use merge values, just push both results to the vector

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

But we do need to change this code eventually right? So an assert will help us get an obvious crash and a FIXME will help us audit the code

pengfei updated this revision to Diff 233009.Tue, Dec 10, 12:47 AM
pengfei marked an inline comment as done.

Revert changes to CondCodeAction.
X86 doesn't need to separate the signalling actions from quiet.

llvm/include/llvm/CodeGen/TargetLowering.h
2099 ↗(On Diff #232850)

Got it. Thanks! I'll revert them.

pengfei updated this revision to Diff 233012.Tue, Dec 10, 1:10 AM

Address review comments.

pengfei marked 2 inline comments as done.Tue, Dec 10, 1:13 AM
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3557

Done. Does it matter for the push order?

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

Right! I added it back. Thanks!

pengfei updated this revision to Diff 233014.Tue, Dec 10, 1:21 AM
pengfei marked an inline comment as done.

Fix the push order issue.

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

Oh, it does matter.

signaling->signalling

Not to be too nitpicky, but I've been using the US spelling (with one 'l') in the common code parts of the patch :-) Should we be consistent here?

pengfei updated this revision to Diff 233045.Tue, Dec 10, 4:06 AM

Change signalling to signaling to follow common code and IEEE754

signaling->signalling

Not to be too nitpicky, but I've been using the US spelling (with one 'l') in the common code parts of the patch :-) Should we be consistent here?

I found IEEE754 uses signaling, I think it's better to follow it as well as common code.

craig.topper accepted this revision.Tue, Dec 10, 1:15 PM

signaling->signalling

Not to be too nitpicky, but I've been using the US spelling (with one 'l') in the common code parts of the patch :-) Should we be consistent here?

I found IEEE754 uses signaling, I think it's better to follow it as well as common code.

Agreed. Sorry for the trouble.

LGTM

This revision is now accepted and ready to land.Tue, Dec 10, 1:15 PM
This revision was automatically updated to reflect the committed changes.