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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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;
----------------
AMDGPU basically already has this, but it requires a depth argument similar to computeKnownBits.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:834
 
+// Return true if the Opcode will treat denormals as zero (DAZ).
+bool AMDGPUTargetLowering::willCanonicalize(SelectionDAG &DAG, SDNode *N) const {
----------------
We're already doing this, but I'm made somewhat uncomfortable by how constant folding is done. We don't insert a canonicalize when constant folding, so if you check isCanonicalized(x), but x is constant folded away into something that should have flushed, this won't be quite right. I guess the way it's defined, this only matters when folding canonicalize inputs?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:846
+  case ISD::FMAD:
+  case ISD::FMAXNUM:
+  case ISD::FP_EXTEND:
----------------
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


================
Comment at: llvm/test/CodeGen/AMDGPU/fneg-combines.ll:11
 
+; FIXME: I think we want to test FNEG(X) folding here. The FSUB(-0,X) case is
+;        uninteresting. Unless these tests should be split into
----------------
Correct, this most of these are for source modifier folding purposes only


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

https://reviews.llvm.org/D73978