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

Tue Dec 10 00:52:37 PST 2019

pengfei added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2099
+                         LegalizeAction Action,
+                         ActionIndex AI = ActionIndex::Both) {
+    assert(VT.isValid() && (unsigned)CC < array_lengthof(CondCodeActions[0]) &&
----------------
craig.topper wrote:
> pengfei wrote:
> > craig.topper wrote:
> > > pengfei wrote:
> > > > craig.topper wrote:
> > > > > 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.
> > > > Yes. We are using it for CCs `SETOEQ` and `SETUNE` in all scalar FP types.
> > > But I couldn't find any modified calls to setCondCodeAction, so aren't we just always passing "Both"?
> > 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.
> 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.
Got it. Thanks! I'll revert them.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70582