[PATCH] D77319: [DAGCombine] Remove the getNegatibleCost to avoid the out of sync with getNegatedExpression

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang marked 2 inline comments as done.
steven.zhang added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:260
   enum class NegatibleCost {
-    Expensive = 0,  // Negated expression is more expensive.
-    Neutral = 1,    // Negated expression has the same cost.
-    Cheaper = 2     // Negated expression is cheaper.
+    Cheaper = 0,  // Negated expression is cheaper.
+    Neutral = 1,  // Negated expression has the same cost.
----------------
qiucf wrote:
> This change can be a parent commit since it's not related to this refactoring.
Done in https://reviews.llvm.org/D77993


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3554
+                                      unsigned Depth = 0) const {
+    NegatibleCost Cost = NegatibleCost::Expensive;
+    SDValue V =
----------------
spatel wrote:
> Do we need to initialize the output value here (and other parts of the patch)? I think it would be better to just document in the code comment for this function that the Cost value is invalid if no SDValue is returned. Alternatively, we could add an 'Invalid' enum value if that's really a concern.
It makes sense. I will remove this line and add comments that the cost value is undefined if no SDValue returned.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12397
+            N1, DAG, LegalOperations, ForCodeSize))
+      return DAG.getNode(ISD::FSUB, DL, VT, N0, NegN1, Flags);
 
----------------
RKSimon wrote:
> Would it help to add getCheaperNegatedExpression as a helper with the existing methods right away and rebase this on top of that?
Good suggestion. I will do that.


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

https://reviews.llvm.org/D77319