[PATCH] D65226: [Strict FP] Allow custom operation actions

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand marked an inline comment as done.
uweigand added a comment.

In D65226#1600009 <https://reviews.llvm.org/D65226#1600009>, @cameron.mcinally wrote:

> In D65226#1599743 <https://reviews.llvm.org/D65226#1599743>, @kpn wrote:
>
> > It looks like we're unrolling some vectors where before we weren't. That seems unfortunate. Is that the reason for the generated code quality regressions?
>
>
> +1. What's the reason behind the scalarization?


Those are cases where the non-strict operation actually is not legal: e.g. the X86 target sets the operation action for FADD and FSUB vector operations to Custom.   The old code simply ignored that and just emitted FADD and FSUB anyway as if they were legal, and only by chance did they match an isel pattern anyway.

The target can get the old behavior back by simply handling STRICT_FADD and STRICT_FSUB directly (presumably also via Custom operations similar to ones it uses for FADD/FSUB), so this is only a "regression" for the current fallback implementation.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2790
+        == TargetLowering::Legal)
+      break;
     Tmp1 = EmitStackConvert(Node->getOperand(1), 
----------------
kpn wrote:
> In what way does it not honor the strict properties?
> 
> Also, wouldn't it be a bug if we were asked to expand a strict node when the non-strict is legal?
> In what way does it not honor the strict properties?

Well, the expansion is done by using a truncating store followed by a load.  The truncating store is a non-strict operation which will not raise exceptions.

> Also, wouldn't it be a bug if we were asked to expand a strict node when the non-strict is legal?

Why would that be a bug?  That's the current default behavior on all targets that don't (yet) support strict operations.   (All strict operations default to expand, which is taken to mean replace by the non-strict version.)



Repository:
  rL LLVM

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

https://reviews.llvm.org/D65226