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

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 1 13:35:10 PDT 2019


spatel marked an inline comment as done.
spatel 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
----------------
arsenm wrote:
> 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?)
I don't object to assuming IEEE by default, but we'll want to at least get the known x86 platforms right in clang before fixing this. I don't know of any comprehensive list that would tell us which platforms are compliant or not (or vary based on some semi-external factor like x86). 

I'm pretty sure there are out-of-tree GPU targets that are always non-compliant (always flush), so we probably need to post to llvm-dev and see what the various target owners want to do.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42323





More information about the llvm-commits mailing list