[PATCH] D60982: [SimplifyCFG] Use lookup tables when they are more space efficient or a huge speed win.

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5183
+
+  // The table density should be at least 33% for 64-bit integers.
   // FIXME: Find the best cut-off.
----------------
shawnl wrote:
> jmolloy wrote:
> > But why? You've replaced 40% with 33% and you mention 64-bit integers but the following heuristic doesn't use 64-bit integers anywhere.
> Along with the comment on the multiplication overflow above: this is basic grade-school multiplication. 1/3 == 33 %; 64 / 8 == 8.
> 
> The heuristic has threatened to de-rail this patch set with bike-shedding, and has made me really frustrated.
> 
> Communication is a key part of doing this work, but when you worry that multiplication will not be understood by the reviewer it really throws a wrench in the process.
Hi Shawn,

I apologise that my review has made you frustrated. If you wish to find a different reviewer, that's fine by me.

I have approximate knowledge of many things. Luckily multiplication is one of them. Clairvoyance is not, and the point of a code review is to ensure that submitted code can be comprehended by anyone, not just its author.

Communication is important as you say, and it's possible that I've been poor in my own communication. My intent with the "But why?" comments wasn't to say "I cannot understand this at all", instead to say "It took me longer than it should to understand this.". The latter does not mean the code is wrong, it means it either requires a comment or refactoring to make the intended logic (not the eventual mechanics) clear.

My view on code in heuristics is that it should be crystal clear what the intent is without any mental gymnastics.

If we take the code as you've rewritten it:

    // The table density should be at least 1/3rd (33%) for 64-bit integers.
    // This is a guess. FIXME: Find the best cut-off.
    return (uint64_t)NumCases * 3 * (64 / 8) >= TableSize * CaseSize;

There are a few things the reader needs to do here. They must realise that the comment describes density with a number (33%), but the code calculates the inverse (denominator on the LHS, numerator on the RHS). It's still not clear to me what the "for 64-bit integers" means in the comment or precisely how it impacts the heuristic function.

Heuristics are always arbitrary, often wrong. They are the trickiest pieces of code to comprehend the intent of because of this, so I pay them more attention.

Also, you changed from 40% to 33% alongside this. Was there a reasoning behind this? People who notice the effects of this change may look back at this review to find a rationale.

Again, apologies if you find this pedantic. I always aim to be anything but pedantic in my reviews.


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

https://reviews.llvm.org/D60982