[PATCH] D42323: [DAGCombiner] filter out denorm inputs when calculating sqrt estimate (PR34994)

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17465
+        Attribute Denorms = F.getFnAttribute("denormal-fp-math");
+        if (Denorms.getValueAsString().equals("ieee")) {
+          // fabs(X) < SmallestNormal ? 0.0 : Est
----------------
spatel wrote:
> arsenm wrote:
> > Why doesn't this assume IEEE behavior if the attribute wasn't specified?
> I had to go back through the comments in:
> https://bugs.llvm.org/show_bug.cgi?id=34994
> 
> The concern was that adding runtime code to the estimate to deal with denorms would reduce performance on platforms (like x86 Linux or PlayStation) that would flush denorms anyway when building with -ffast-math. 
> 
> So we offered the explicit use of clang's "-fdenormal-fp-math=ieee" as a way to specify the seemingly uncommon case. If we say that LLVM assumes an IEEE target by default, then we could change that. But that means we need to explicitly set the function attribute for all targets in clang?
I think it's important to assume ieee behavior by default and there could be a denormal. Is there a comprehensive list of platforms that by default disable denorms for clang to emit flushing (and what type of flushing they use?)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42323