[PATCH] D60522: [KnownBits] Add computeForAddCarry()

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 2 inline comments as done.
nikic added inline comments.


================
Comment at: llvm/lib/Support/KnownBits.cpp:18
 
-KnownBits KnownBits::computeForAddSub(bool Add, bool NSW,
-                                      const KnownBits &LHS, KnownBits RHS) {
-  // Carry in a 1 for a subtract, rather than 0.
-  bool CarryIn = false;
-  if (!Add) {
-    // Sum = LHS + ~RHS + 1
-    std::swap(RHS.Zero, RHS.One);
-    CarryIn = true;
-  }
+static KnownBits computeForAddCarryInternal(
+    const KnownBits &LHS, const KnownBits &RHS,
----------------
lebedev.ri wrote:
> nikic wrote:
> > lebedev.ri wrote:
> > > The signature is already different from `computeForAddCarry()` (2 `bool`s vs 1 `KnownBits`),
> > > i don't see the need for `Internal` suffix.
> > > Also, i believe anonymous namespaces are preferred, so drop `static`, and wrap into `namespace {}`.
> > Agree on the suffix being unnecessary. Regarding anonymous namespaces, https://llvm.org/docs/CodingStandards.html#anonymous-namespaces says:
> > 
> > > Because of this, we have a simple guideline: make anonymous namespaces as small as possible, and only use them for class declarations.
> > 
> > If I'm understanding correctly, this means that anon namespaces should be used for classes and static for functions.
> > If I'm understanding correctly, this means that anon namespaces should be used for classes and static for functions.
> 
> There have been discussions about exactly that recently, in other reviews.
> So i'm not confident what is currently considered as the best common practice for functions.
I'd prefer to leave this as a static function for now -- if there's interest in changing the recommended practice around this it should imho start with a patch to CodingStandards.


================
Comment at: llvm/lib/Support/KnownBits.cpp:54
+
+KnownBits KnownBits::computeForAddSub(bool Add, bool NSW,
+                                      const KnownBits &LHS, KnownBits RHS) {
----------------
lebedev.ri wrote:
> All these `bool` params are potential bugs, and really affect readability.
> But i guess that would be too intrusive for this header.
We might want to start a separate file outside Support (I'm thinking `llvm/Analysis/KnownBitsOps.h` and `lib/Analysis/KnownBitsOps.cpp`) that can depend on things like binop opcodes and obo flags and allow sharing more of the KnownBits code between ValueTracking and SDAG. The existing KnownBits header mostly has only primitive operations and is quite cumbersome to edit (because it rebuilds half of LLVM).


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

https://reviews.llvm.org/D60522