[PATCH] D29524: [DAGCombiner] Make DAGCombiner smarter about overflow

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1283
+
+  OverflowKind computeOverflowKind(SDValue N0, SDValue N1) const;
+
----------------
RKSimon wrote:
> Please document both the enum and the method.
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Style guide says enums should ave a prefix: OFK_Never, OFK_Sometimes, OFK_Always


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1287
+
+  /// Determine if the result o the addition of 2 node can overflow.
+  /// Return Never if it can never overflow, always if it will
----------------
result of


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1290
+  /// always overflow, and sometime in other cases.
+  OverflowKind computeOverflowKind(SDValue N0, SDValue N1) const;
+
----------------
You can probably drop this part of the description now that OverflowKind is documented


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2780
+      (~N1Zero & 0x01) == ~N1Zero)
+    return OverflowKind::Never;
+
----------------
Does the test cases cover both of these patterns? Its not clear if it does.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2782
+
+  return OverflowKind::Sometime;
+}
----------------
Are you intending to add OverflowKind::Always cases soon? If not you should possibly drop it from the enum and just put in a TODO.


https://reviews.llvm.org/D29524