[PATCH] D28200: [BypassSlowDivision] Do not bypass division of hash-like values

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:118
+    const PHINode *P = cast<PHINode>(I);
+    int NumVal = P->getNumIncomingValues();
+    for (int i = 0; i < NumVal; ++i)
----------------
n.bozhenov wrote:
> sanjoy wrote:
> > Why not:
> > 
> > ```
> > bool Found = llvm::any_of(P->incoming_values(), [&](Value *V) {
> >   return isHashLineValue(V, Width, Depth);
> > });
> > 
> > if (Found)
> >   return true;
> > ```
> That's cool! But do you think such code would be really easier to read? :)
I do, because it makes the high-level structure of the program explicit.  That is, rather than saying "do a loop that does some stuff", we say "check if any elements have property X".


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:228
+      return true;
+    break;
+  }
----------------
Perhaps we can write this as

  return C && C->getValue()...

And then below, we can write

  return any_of(P->incoming_values(), [&](Value *V) {
    return isHashLikeValue(V, Width, Depth);
  });

We could even move the `cast` into the any_of call.

  return any_of(cast<PHINode>(I)->incoming_values(), [&](Value *V) {
    return isHashLikeValue(V, Width, Depth);
  });


This is, I think, in the spirit of the LLVM style guide's section about "reducing nesting" and having early returns / breaks.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:265
+  // unlikely to have enough leading zeros. The call below is supposed to detect
+  // values that are unlikely to fit BypassType (including hashes).
+  if (isHashLikeValue(V))
----------------
Maybe s/is supposed to detect/tries to detect/


================
Comment at: test/CodeGen/X86/bypass-slow-division-fnv.ll:3
+; RUN: opt < %s -O3 -unroll-count=4 | llc -mattr=+idivq-to-divl | FileCheck %s
+; CHECK-NOT: divl
+
----------------
Like in the earlier discussion, can we just test this in LLVM IR?

Instead of testing loop unrolling, could we just run your testcase through opt -O3 -unroll-count=4 (maybe we only need unroll-count=2) and check that in?  Then here we'd just need to run codegenprepare and check the output of that.

We prefer for unit tests to test small units of code, instead of running the whole optimization pipeline.  Whole-pipeline tests are better suited for the test-suite.


================
Comment at: test/CodeGen/X86/bypass-slow-division-fnv.ll:10
+; A simple but widely used hash algorithm with hash operations hidden inside a
+; (probably) unrolled loop. The test is supposed to verify that such code is
+; recognized even after running loop unrolling on it and any other applicable
----------------
again s/is supposed to verify/verifies


================
Comment at: test/CodeGen/X86/bypass-slow-division-fnv.ll:12
+; recognized even after running loop unrolling on it and any other applicable
+; optimizations.
+
----------------
Starting with "reorganized", I don't understand what this is saying, or how it meshes with the comment at the top of the file.  Can we harmonize these comments and maybe just have one comment instead of two?


https://reviews.llvm.org/D28200