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

[Nikolai Bozhenov via Phabricator via llvm-commits]

n.bozhenov marked 8 inline comments as done.
n.bozhenov 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)
----------------
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? :)


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:125
+  default:
+    break;
+  }
----------------
jlebar wrote:
> We don't need this case?
My idea was to add an explicit do-nothing default branch because we are not going to cover all possible enumeration values. But it seems not working, because getOpcode() returns a plain unsigned int. I have deleted the default branch.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:159
+  if (isHashLikeValue(Op, shortLen))
+    return VALRNG_LONG;
+
----------------
jlebar wrote:
> We only want to do this check for the dividend, right?  I mean, I guess it doesn't really hurt to do it for the divisor too, but that isn't motivated by hashtables.
Basically, yes. This patch was motivated by hashtables and I expect this check to fire mostly for dividends. However, I tried to make the patch as generic as possible. And if we find a divisor that looks like a hash value, disabling bypassing for such a division operation will be a good idea.


================
Comment at: test/CodeGen/X86/bypass-slow-division-64.ll:138
+; CHECK-NOT: divl
+  %c = mul i64 %a, 5229553307 ; the constant doesn't fit 32 bits
+  %res = urem i64 %c, %l
----------------
jlebar wrote:
> Can we test with a constant that *does* fit into 32 bits?
Isn't the next test what you're talking about?


https://reviews.llvm.org/D28200