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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

The existing code is buggy, and there's general agreement that this is a good way forward, so please make whatever NFC changes (like switching the enum values) are possible as preliminary patches. Also, make small edits that were already suggested (improve code comments, add assert message string). That will reduce risk and make the final review easier.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5719
+      Cost = CostX;
+      assert(Cost != NegatibleCost::Expensive);
+      return DAG.getNode(Opcode, DL, VT, NegX, Y, Flags);
----------------
steven.zhang wrote:
> qiucf wrote:
> > 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.
> Thank you for the review. Yes, as the getNegatedExpression could be override by target, we need to make sure that they didn't return non-null value with expensive cost, as we make assumption that, if the return value is null, it means expensive negating.
Add an assert string for this and other assert lines (if they are going to remain):
http://llvm.org/docs/CodingStandards.html#assert-liberally


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

https://reviews.llvm.org/D77319