This is a follow up of D69281, it enables the X86 backend support for the FP comparision.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 42197 Build 42612: arc lint + arc unit
Event Timeline
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. |
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. | |
3665 | Same as above. |
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. |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
3595 | We can enhance setCondCodeAction, but it's hard to enhance the expansion math here. |
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! |
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 | ||
20293 | IsSignaling->IsSignalling | |
20866 | singaling->signalling | |
20914 | signaling->signalling | |
20926 | Why is this change needed? | |
21313 | Space before : | |
21317 | Please run clang-format on this. I think the = should be at the end of the line above | |
21322 | Is this something we need to fix? if so put a fixme here | |
21477 | 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 |
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 | ||
20926 | They have the same meaning here. Changes reverted. | |
21322 | Removed. | |
llvm/test/CodeGen/X86/fp80-strict-scalar-cmp.ll | ||
4 | Removed, thanks! |
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"? |
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. |
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. |
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. |
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?
I found IEEE754 uses signaling, I think it's better to follow it as well as common code.
bug report
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
3565 | Got a bug report like this, @pengfei can you please fix this as soon as possible? [tag] [reply] [−]DescriptionMelanie Blower 2020-10-27 09:29:07 PDT After D87528: Enable '#pragma STDC FENV_ACCESS' commit, our bots have a failure on the wasm target, could you please take a look? test.c: double a, b; c() { #pragma STDC FENV_ACCESS ON b == a; } Run clang -cc1 -triple wasm32-unknown-wasi -emit-obj test.c (clang needs to be build with -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=WebAssembly) Result: clang: /home/vm/src/llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3764: bool (anonymous namespace)::SelectionDAGLegalize::ExpandNode(llvm::SDNode *): Assertion `!IsStrict && "Don't know how to expand for strict nodes."' failed. |
Can we add to the Results vector here instead of doing manual replacement?