[PATCH] D70582: [FPEnv][X86] Constrained FCmp intrinsics enabling on X86

Thu Dec 5 20:06:57 PST 2019

pengfei marked an inline comment as done.
pengfei added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3577
     EVT VT = Node->getValueType(0);
+    SDValue Chain;
     SDValue CC = Node->getOperand(4);
----------------
craig.topper wrote:
> pengfei wrote:
> > uweigand wrote:
> > > 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 ...
> > 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.
> 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70582/new/

https://reviews.llvm.org/D70582