[PATCH] D73978: [WIP][FPEnv] Don't transform FSUB(-0.0,X)->FNEG(X) when flushing denormals

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as done.
cameron.mcinally added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:464
+  /// Return true if denormals will be flushed to zero.
+  virtual bool willCanonicalize(SelectionDAG &DAG, SDNode *N) const {
+    return false;
----------------
arsenm wrote:
> AMDGPU basically already has this, but it requires a depth argument similar to computeKnownBits.
I did see isCanonicalized(...), but it looks like it goes the other direction. I.e.  isCanonicalized(...) checks to see if the predecessor is already canonicalized. willCanonicalize(...) checks to see if the successors will canonicalize the result of the operation. There are a number of existing tests that begin with an FSUB(-0, X), so that's why I choose this solution.

I think we'd eventually want both directions, for completeness. But I noticed that isCanonicalized(...) only exists in SIISelLowering, and I didn't want to mess around with something I didn't fully understand.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:846
+  case ISD::FMAD:
+  case ISD::FMAXNUM:
+  case ISD::FP_EXTEND:
----------------
arsenm wrote:
> Weird to have FMAXNUM but not FMINNUM. I also think we have a defective implementation for subtargets where the instructions don't read the FP mode. We inspect the inputs of the generic node rather than introducing a target specific wrapper with the broken behavior
Agreed. This switch is only opcodes that existed in current testing, so there are some gaps. I should probably add FMINNUM under a separate patch.

That said, I could introduce a test case pre-commit and then fix it in this patch. That's probably the right way to go forward.

This switch is also likely incorrect at the edges (e.g. FMED3, FMA). I don't fully understand all the intricacies of AMDGPU flushing [as seen in isCanonicalized(...)]. There's more work needed here.


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

https://reviews.llvm.org/D73978