[HashRecognize] Make it a non-PM analysis

[artagnon]

Make HashRecognize a non-PassManager analysis that can be called to get the result on-demand, creating a new getResult() entry-point. The issue was discovered when attempting to use the analysis to perform a transform in LoopIdiomRecognize.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Ramkumar Ramachandra (artagnon)

Changes

Make the analysis return a proper analysis result that can be cached, instead of a HashRecognize object which has to be called to perform the actual analysis. This also means that we need a function_ref to generate the Sarwate table returned along with the analysis result. The issue was discovered when attempting to use the analysis to perform a transform.

---

Full diff: https://github.com/llvm/llvm-project/pull/144742.diff

2 Files Affected:

• (modified) llvm/include/llvm/Analysis/HashRecognize.h (+6-5)
• (modified) llvm/lib/Analysis/HashRecognize.cpp (+18-12)

diff --git a/llvm/include/llvm/Analysis/HashRecognize.h b/llvm/include/llvm/Analysis/HashRecognize.h
index 8ab68a5dc2cb1..8aa1e0e788f99 100644
--- a/llvm/include/llvm/Analysis/HashRecognize.h
+++ b/llvm/include/llvm/Analysis/HashRecognize.h
@@ -66,6 +66,10 @@ struct PolynomialInfo {
   // Set to true in the case of big-endian.
   bool ByteOrderSwapped;
 
+  // A function_ref to generate the Sarwate lookup-table, which can be used to
+  // optimize CRC in the absence of target-specific instructions.
+  function_ref<CRCTable(const APInt &, bool)> GenSarwateTable;
+
   // An optional auxiliary checksum that augments the LHS. In the case of CRC,
   // it is XOR'ed with the LHS, so that the computation's final remainder is
   // zero.
@@ -73,6 +77,7 @@ struct PolynomialInfo {
 
   PolynomialInfo(unsigned TripCount, const Value *LHS, const APInt &RHS,
                  const Value *ComputedValue, bool ByteOrderSwapped,
+                 function_ref<CRCTable(const APInt &, bool)> GenSarwateTable,
                  const Value *LHSAux = nullptr);
 };
 
@@ -87,10 +92,6 @@ class HashRecognize {
   // The main analysis entry point.
   std::variant<PolynomialInfo, ErrBits, StringRef> recognizeCRC() const;
 
-  // Auxilary entry point after analysis to interleave the generating polynomial
-  // and return a 256-entry CRC table.
-  CRCTable genSarwateTable(const APInt &GenPoly, bool ByteOrderSwapped) const;
-
   void print(raw_ostream &OS) const;
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
@@ -113,7 +114,7 @@ class HashRecognizeAnalysis : public AnalysisInfoMixin<HashRecognizeAnalysis> {
   static AnalysisKey Key;
 
 public:
-  using Result = HashRecognize;
+  using Result = std::optional<PolynomialInfo>;
   Result run(Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR);
 };
 } // namespace llvm
diff --git a/llvm/lib/Analysis/HashRecognize.cpp b/llvm/lib/Analysis/HashRecognize.cpp
index 1edb8b3bdc9a8..83a0c36451949 100644
--- a/llvm/lib/Analysis/HashRecognize.cpp
+++ b/llvm/lib/Analysis/HashRecognize.cpp
@@ -442,11 +442,14 @@ getRecurrences(BasicBlock *LoopLatch, const PHINode *IndVar, const Loop &L) {
   return std::make_pair(SimpleRecurrence, ConditionalRecurrence);
 }
 
-PolynomialInfo::PolynomialInfo(unsigned TripCount, const Value *LHS,
-                               const APInt &RHS, const Value *ComputedValue,
-                               bool ByteOrderSwapped, const Value *LHSAux)
+PolynomialInfo::PolynomialInfo(
+    unsigned TripCount, const Value *LHS, const APInt &RHS,
+    const Value *ComputedValue, bool ByteOrderSwapped,
+    function_ref<CRCTable(const APInt &, bool)> GenSarwateTable,
+    const Value *LHSAux)
     : TripCount(TripCount), LHS(LHS), RHS(RHS), ComputedValue(ComputedValue),
-      ByteOrderSwapped(ByteOrderSwapped), LHSAux(LHSAux) {}
+      ByteOrderSwapped(ByteOrderSwapped), GenSarwateTable(GenSarwateTable),
+      LHSAux(LHSAux) {}
 
 /// In the big-endian case, checks the bottom N bits against CheckFn, and that
 /// the rest are unknown. In the little-endian case, checks the top N bits
@@ -471,8 +474,7 @@ static bool checkExtractBits(const KnownBits &Known, unsigned N,
 /// Generate a lookup table of 256 entries by interleaving the generating
 /// polynomial. The optimization technique of table-lookup for CRC is also
 /// called the Sarwate algorithm.
-CRCTable HashRecognize::genSarwateTable(const APInt &GenPoly,
-                                        bool ByteOrderSwapped) const {
+static CRCTable genSarwateTable(const APInt &GenPoly, bool ByteOrderSwapped) {
   unsigned BW = GenPoly.getBitWidth();
   CRCTable Table;
   Table[0] = APInt::getZero(BW);
@@ -625,7 +627,7 @@ HashRecognize::recognizeCRC() const {
 
   const Value *LHSAux = SimpleRecurrence ? SimpleRecurrence.Start : nullptr;
   return PolynomialInfo(TC, ConditionalRecurrence.Start, GenPoly, ComputedValue,
-                        *ByteOrderSwapped, LHSAux);
+                        *ByteOrderSwapped, genSarwateTable, LHSAux);
 }
 
 void CRCTable::print(raw_ostream &OS) const {
@@ -679,7 +681,7 @@ void HashRecognize::print(raw_ostream &OS) const {
     OS << "\n";
   }
   OS.indent(2) << "Computed CRC lookup table:\n";
-  genSarwateTable(Info.RHS, Info.ByteOrderSwapped).print(OS);
+  Info.GenSarwateTable(Info.RHS, Info.ByteOrderSwapped).print(OS);
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
@@ -693,13 +695,17 @@ PreservedAnalyses HashRecognizePrinterPass::run(Loop &L,
                                                 LoopAnalysisManager &AM,
                                                 LoopStandardAnalysisResults &AR,
                                                 LPMUpdater &) {
-  AM.getResult<HashRecognizeAnalysis>(L, AR).print(OS);
+  HashRecognize(L, AR.SE).print(OS);
   return PreservedAnalyses::all();
 }
 
-HashRecognize HashRecognizeAnalysis::run(Loop &L, LoopAnalysisManager &AM,
-                                         LoopStandardAnalysisResults &AR) {
-  return {L, AR.SE};
+std::optional<PolynomialInfo>
+HashRecognizeAnalysis::run(Loop &L, LoopAnalysisManager &AM,
+                           LoopStandardAnalysisResults &AR) {
+  auto Res = HashRecognize(L, AR.SE).recognizeCRC();
+  if (std::holds_alternative<PolynomialInfo>(Res))
+    return std::get<PolynomialInfo>(Res);
+  return std::nullopt;
 }
 
 AnalysisKey HashRecognizeAnalysis::Key;

[nikic]

Have you considered going the other way around and not making this a PM-level analysis at all? You can just locally instantiate the object in the single place you're going to use it. I don't think you gain anything but unwelcome problems (like: did the results potentially get invalidated by a LIR transform that ran earlier?)

[artagnon]

Have you considered going the other way around and not making this a PM-level analysis at all? You can just locally instantiate the object in the single place you're going to use it. I don't think you gain anything but unwelcome problems (like: did the results potentially get invalidated by a LIR transform that ran earlier?)

I thought about that, yes. I did lean towards making it a PM-level analysis for ease of testing. I leverage the existing infrastructure in update_analyze_test_checks to auto-generate the result, with a simple opt invocation in the RUN line. If this weren't a PM-level analysis, I'd have to hand-craft unit-tests in C++.

I think the specific problem you mentioned can be easily tackled by not preserving the analysis result in LIR. I don't think LIR runs more than once anyway, so I don't think it's ever computed more than once.

Besides, I plan to grow HashRecognize in size by also including other polynomial hashes, and possibly requiring more analysis results. In that sense, I think it is less like ConstraintSystem, and more like an independent analysis.

[artagnon]

I thought about that, yes. I did lean towards making it a PM-level analysis for ease of testing. I leverage the existing infrastructure in update_analyze_test_checks to auto-generate the result, with a simple opt invocation in the RUN line. If this weren't a PM-level analysis, I'd have to hand-craft unit-tests in C++.

I think the specific problem you mentioned can be easily tackled by not preserving the analysis result in LIR. I don't think LIR runs more than once anyway, so I don't think it's ever computed more than once.

Besides, I plan to grow HashRecognize in size by also including other polynomial hashes, and possibly requiring more analysis results. In that sense, I think it is less like ConstraintSystem, and more like an independent analysis.

Actually, for testing, I only need the Printer pass. I thought about it some more, and I think it would make sense to strip the analysis pass entirely, and just pass SE and L for now. If we need more things, we can always get it in LIR, and pass it: I don't expect this to be used outside LIR anyway.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nikic]

LGTM