[opt] Simplify carry extraction pattern (prepending zero bit, adding, then slicing above the original MSb)

[cdleary]

Note the "ZeroExtendedBitsView" in this change seems useful until we come to stronger conclusions on canonical forms (i.e. zero_ext I think should be more canonical than concat as it's a more precise operator, but wouldn't want to make that a prereq).

• Match: bit_slice(add(zext(v), k), start=N, width=1) (carry-out of v + k).
• Rewrite: bit_slice(...) becomes (k[N] ? ult(v, (2^N - (k mod 2^N))) : uge(v, (2^N - (k mod 2^N)))) (and if (k mod 2^N) == 0, becomes literal(k[N])).

before:

$ bazel-bin/xls/tools/opt_main ~/missed_cone.ir 
package cone

top fn cone(leaf_193: bits[8] id=1, leaf_237: bits[1] id=2, leaf_466: bits[1] id=3) -> bits[1] {
  literal.4: bits[1] = literal(value=0, id=4)
  not.6: bits[8] = not(leaf_193, id=6)
  concat.7: bits[9] = concat(literal.4, not.6, id=7)
  literal.8: bits[9] = literal(value=127, id=8)
  add.9: bits[9] = add(concat.7, literal.8, id=9)
  literal.5: bits[8] = literal(value=255, id=5)
  bit_slice.10: bits[1] = bit_slice(add.9, start=8, width=1, id=10)
  eq.20: bits[1] = eq(leaf_193, literal.5, id=20)
  not.13: bits[1] = not(leaf_237, id=13)
  ret and.30: bits[1] = and(bit_slice.10, eq.20, leaf_466, not.13, id=30)
}

which is:

$ ~/proj/xlsynth-crate/target/release/xlsynth-driver ir2gates ~/missed_cone.ir --quiet true
{"live_nodes":28,"deepest_path":7,...}

after:

$ bazel-bin/xls/tools/opt_main ~/missed_cone.ir 
package cone

top fn cone(leaf_193: bits[8] id=1, leaf_237: bits[1] id=2, leaf_466: bits[1] id=3) -> bits[1] {
  ret literal.35: bits[1] = literal(value=0, id=35)
}

[allight]

FWIW actually concat is currently considered the 'canonical' representation of zero extend (

xls/xls/passes/canonicalization_pass.cc

Line 257 in c891969

if (n->op() == Op::kZeroExt) {

). I agree this might be something we want to change though.

Anyway mostly minor things and looks good otherwise.

[allight]

Probably want to just move the ZeroExtLike from reassociation_pass.cc into node-util and use that instead

xls/xls/passes/reassociation_pass.cc

Line 74 in c891969

class ZeroExtLike {

[allight]

nit:

IMO something more general like:

template <typename T, typename Func0, typename... Funcs>
  requires(std::is_swappable_v<T> && std::is_invocable_r_v<bool, Func0, T>)
bool MatchingOrderSpan(absl::Span<T> arr, Func0 f0, Funcs... funcs) {
  for (int i = 0; i < arr.size(); ++i) {
    std::swap(arr[0], arr[i]);
    if (f0(arr[0]) &&
        MatchingOrderSpan(arr.subspan(1), std::forward<Funcs>(funcs)...)) {
      return true;
    }
    std::swap(arr[0], arr[i]);
  }
  return false;
}
template <typename T, typename Func0>
  requires(std::is_invocable_r_v<bool, Func0, T>)
bool MatchingOrderSpan(absl::Span<T> arr, Func0 f0) {
  if (arr.size() != 1) {
    return false;
  }
  return f0(arr[0]);
}
template <typename T>
bool MatchingOrderSpan(absl::Span<T> arr) {
  if (arr.size() != 0) {
    return false;
  }
  return true;
}

template <typename T, typename... Funcs>
std::optional<std::array<T, std::tuple_size_v<std::tuple<Funcs...>>>>
MatchingOrder(std::array<T, std::tuple_size_v<std::tuple<Funcs...>>> arr,
              Funcs... funcs) {
  if (MatchingOrderSpan(absl::MakeSpan(arr), std::forward<Funcs>(funcs)...)) {
    return arr;
  }
  return std::nullopt;
}

TEST(MatchingOrderTest, Works) {
  std::array<int, 3> arr = {1, 2, 3};
  auto v = MatchingOrder(
      arr, [](int i) { return i == 3; }, [](int i) { return i == 2; },
      [](int i) { return i == 1; });
  EXPECT_THAT(v, testing::Optional(testing::_));
  EXPECT_EQ(*v, (std::array<int, 3>{3, 2, 1}));
}
TEST(MatchingOrderTest, Works2) {
  auto v = MatchingOrder<int>(
      {4, 2, 3}, [](int i) { return i == 3; }, [](int i) { return i == 2; },
      [](int i) { return i == 1; });
  EXPECT_FALSE(v);
}

in xls/common would be a bit preferable.

In any event I think its probably a good idea to move away from callbacks like on_match since they make the control flow hard to follow.

[allight]

I don't understand what this is trying to show. AFAICT it just makes the test longer? Remove.

[allight]

Don't give tests names with _. CarryOutOfAddWithZeroExtendedOperandMatchesSwappedAddOperands

This and all of the other tests.

[allight]

see above

[proppy]

@cdleary, did you get a chance to take a look at @allight comments?