[PATCH] D24033: Convert clamp into fmaxnum/fminnum pairs.

[Matt Arsenault via llvm-commits]

arsenm added a subscriber: arsenm.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5106-5107
@@ +5105,4 @@
+  // Check if num of operands match requirements
+  if (CmpNode->getNumOperands() < 2
+      || N2->getNumOperands() < 2
+      || CmpNode->getOpcode() != ISD::SETCC
----------------
The || should go at the end of the line

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5106-5107
@@ +5105,4 @@
+  // Check if num of operands match requirements
+  if (CmpNode->getNumOperands() < 2
+      || N2->getNumOperands() < 2
+      || CmpNode->getOpcode() != ISD::SETCC
----------------
arsenm wrote:
> The || should go at the end of the line
Why are you checking the number of operands? You can just check that it's SETCC

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5109
@@ +5108,3 @@
+      || CmpNode->getOpcode() != ISD::SETCC
+      || CmpNode->getOperand(0).getNode() != N2.getOperand(0).getNode())
+    return SDValue();
----------------
Don't need the get node (and actually might be broken)

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5113
@@ +5112,3 @@
+  ConstantFPSDNode* CmpN1FP =
+    dyn_cast<ConstantFPSDNode>(CmpNode->getOperand(1).getNode());
+  ConstantFPSDNode* N1FP = dyn_cast<ConstantFPSDNode>(N1);
----------------
You don't need the get node

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5115
@@ +5114,3 @@
+  ConstantFPSDNode* N1FP = dyn_cast<ConstantFPSDNode>(N1);
+  if (!CmpN1FP || !N1FP) { return SDValue(); }
+
----------------
Return on separate line

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5120-5123
@@ +5119,6 @@
+
+  // Check if float point constant are the same
+  if (&CmpN1FPVal.getSemantics() != &N1FPVal.getSemantics()
+      || CmpN1FPVal.compare(N1FPVal) != APFloat::cmpEqual)
+    return SDValue();
+
----------------
You don't need this, the DAG is broken if this ever happens

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5134-5135
@@ +5133,4 @@
+  APFloat N2Operand2Val = N2Operand2->getValueAPF();
+  if (&CmpN1FPVal.getSemantics() != &N2Operand2Val.getSemantics())
+    return SDValue();
+
----------------
Ditto

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5139
@@ +5138,3 @@
+  // Switch based on the comparison operand.
+  switch (dyn_cast<CondCodeSDNode>(CmpNode->getOperand(2))->get()) {
+  case ISD::SETOLT:
----------------
unchecked dynast, used cast<>

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5353-5354
@@ -5263,1 +5352,4 @@
+      //     t10 = fminnum t9, f1
+      if (isa<ConstantFPSDNode>(N1) && N0->getOpcode() == ISD::SETCC)
+        return combineSelectFP(N);
     }
----------------
This should probably only return if combineSelectFP succeeded 

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-DAGCombine-fminmax.ll:1
@@ +1,2 @@
+; RUN: llc --enable-unsafe-fp-math -O=3 -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck %s
+; Make sure we are not crashing on this test.
----------------
Don't need -O=3

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-DAGCombine-fminmax.ll:1
@@ +1,2 @@
+; RUN: llc --enable-unsafe-fp-math -O=3 -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck %s
+; Make sure we are not crashing on this test.
----------------
arsenm wrote:
> Don't need -O=3
This needs to be run without unsafe math to make sure it isn't happening without it

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-DAGCombine-fminmax.ll:2
@@ +1,3 @@
+; RUN: llc --enable-unsafe-fp-math -O=3 -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck %s
+; Make sure we are not crashing on this test.
+
----------------
Comment is wrong

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-DAGCombine-fminmax.ll:5
@@ +4,3 @@
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
This is redundant with the run line and should be removed

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-DAGCombine-fminmax.ll:53
@@ +52,3 @@
+}
+
+attributes #0 = { norecurse nounwind readnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="true" "no-jump-tables"="false" "no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="true" "use-soft-float"="false" }
----------------
Missing a testcase where there are the minnum/maxnum intrinsic calls instead of ones formed from unsafe cmp + select

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-DAGCombine-fminmax.ll:54
@@ +53,3 @@
+
+attributes #0 = { norecurse nounwind readnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="true" "no-jump-tables"="false" "no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="true" "use-soft-float"="false" }
+
----------------
Extra string attributes should be removed

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-DAGCombine-fminmax.ll:56-58
@@ +55,4 @@
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 4.0.0 "}
----------------
Should be removed


Repository:
  rL LLVM

https://reviews.llvm.org/D24033