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

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added a comment.

This patch overall looks like good refactoring, which resolves the '//inconsistency//' and redundant logic between two methods.

Test changes make sense here since we have no way to tell combiner that 'these nodes are totally useless'. If we really want to keep tests unchanged, we need to add some hint to `getNode` or worklist handling logic.



================
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.
----------------
This change can be a parent commit since it's not related to this refactoring.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5719
+      Cost = CostX;
+      assert(Cost != NegatibleCost::Expensive);
+      return DAG.getNode(Opcode, DL, VT, NegX, Y, Flags);
----------------
If I understand correctly, we can make sure only internal bug in this method would hit the assert? Since we shouldn't get non-null `NegX` but expensive `CostX`.

I just mean: we used to have many asserts in this function, because we need to ensure it's 'aligned' with return value of `getNegatibleCost`. But now we removed that function and use null SDValue to represent `Expensive` results, so we don't need those asserts any more.

Only asserts like here to catch internal bugs are okay.


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

https://reviews.llvm.org/D77319