[PATCH] D61160: [SimplifyCFG] Implement the suggested ctlz transform

[James Molloy via Phabricator via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5520
+  bool Cttz = false;
+  while (true) {
+    bool GotCollision = false;
----------------
This loop is scary and hard to read. It's really asking for a helper function. I would do something like this:

  // Given a set of equivalence classes, return true if, for every pair A, B in
  // Values:
  //   Fn(A) == Fn(B) if and only if EC(A) == EC(B).
  template <typename Function>
  bool IsBijectiveOverEquivalenceClasses(const EquivalenceClasses<APInt> &Values,
                                         const Function &Fn) {
    DenseMap<unsigned, APInt> Leaders;
    for (auto &V : Values) {
      auto TransformedValue = Fn(V.getData());
      auto LeaderIt = Leaders.find(TransformedValue);
      if (LeadersIt == Leaders.end()) {
        Leaders[TransformedValue] = V.getLeader()->getData();
        continue;
      }
      if (LeaderIt->getData() != V.getData())
        // This transformed value has already been seen and did not belong to the
        // same equivalence class. This is not a bijective relation.
        return false;
    }
    return true;
  }

  static bool CanReduceSwitchRangeWithCtlzOrCttz(SwitchInst &SI, bool &UseCtlz,
                                                 bool &UseCttz, bool &UseInvert) {
    // Preconstruct equivalence classes of values based on successor block.
    DenseMap<BasicBlock *, APInt> Last;
    EquivalenceClasses<APInt> ECs;
    for (auto CI : SI.cases()) {
      auto I = Last.try_emplace(CI.getCaseSuccessor(), CI.getCaseValue()).first;
      ECs.unionSets(I.second, V);
    }
    UseInvert = false;

    UseCtlz = true;
    if (IsBijectiveOverEquivalenceClasses(
            ECs, [&](const APInt &V) { return V.countLeadingZeros(); }))
      return true;

    UseCtlz = false;
    if (IsBijectiveOverEquivalenceClasses(
            ECs, [&](const APInt &V) { return V.countTrailingZeros(); }))
      return true;

    UseInvert = true;
    UseCtlz = true;
    if (IsBijectiveOverEquivalenceClasses(
            ECs, [&](const APInt &V) { return (~V).countLeadingZeros(); }))
      return true;

    UseCtlz = false;
    if (IsBijectiveOverEquivalenceClasses(
            ECs, [&](const APInt &V) { return (~V).countTrailingZeros(); }))
      return true;

    return false;
  }

  static bool ReduceSwitchRangeWithCtlzOrCttz(SwitchInst &SI,
                                              SmallVectorImpl<uint64_t> &Values,
                                              bool &UseCtlz, bool &UseCttz,
                                              bool &UseInvert) {
    // Note we don't pass Values into this function. This predicate uses the
    // switch's current case values *and* their associated successor block, so a
    // rearranged Values array cannot be analyzed (because we wouldn't know which
    // successsor belonged to which value).
    if (!CanReduceSwitchRangeWithCtlzOrCttz(SI, UseCtlz, UseCttz, UseInvert))
      return false;

    for (auto CI : SI.cases()) {
      APInt I = CI.getCaseValue();
      if (UseCtlz)
        Values.push_back((Invert ? ~V : V).countTrailingZeros());
      else
        Values.push_back((Invert ? ~V : V).countLeadingZeros());
    }
    llvm::sort(Values);
    return true;
  }



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5532
+      if (Got & (1ULL << Ctz)) {
+        if (&*SI.findCaseValue(ConstantInt::get(
+                SI.getContext(), Invert ? ~Int : Int)) == Prev[Ctz])
----------------
This is a fallacy (will always be false) because:

  # SwitchInst::CaseHandle is unique based on switched value. There is one CaseHandle for every value in Values.
  # Prev is zero-initialized just above this line, in the loop.


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

https://reviews.llvm.org/D61160