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

[Hans Wennborg via Phabricator via llvm-commits]

hans 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.
----------------
jmolloy wrote:
> 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.
To give some background, the 40% density was originally chosen to match the density used to form switch jump tables in SelectionDAGBuilder. That density is still used for -Os builds, see OptsizeJumpTableDensity in TargetLoweringBase.cpp.

Changing the threshold here may well be a good idea, especially since it has dropped to 10% for non-optsize jump tables  (JumpTableDensity in TargetLoweringBase.cpp), but such a change needs at least a comment with the motivation.

Perhaps it could be synced with the jump table density? Or perhaps it could be left alone for now -- there's already a lot going on in this patch.

Currently you're returning false for optsize functions. If a switch is >40% dense it means we will instead build a jump table, which is likely to be larger so this would be both a size and performance regression.


Regarding the "randomly dividing by 10" before, that's my fault too.

The code currently looks like:

```
  // The table density should be at least 40%. This is the same criterion as for
  // jump tables, see SelectionDAGBuilder::handleJTSwitchCase.
  // FIXME: Find the best cut-off.
  return SI->getNumCases() * 10 >= TableSize * 4;
```

If I were to write that today I might have spelled it out with *100 and *40 instead to make it really clear that 40 is a percentage, but I optimized prematurely.

The "TableSize >= UINT64_MAX / 10" check above is to protect against overflow, as the comment says, but as the code evolved and that line moved further away from the density computation, it became harder to make the connection.



I mentioned before that I have trouble following along with this patch series. This one has a title of "Use lookup tables when they are more space efficient or a huge speed win" which is pretty vague, and the inline description says things like "trying to move towards much more space-efficient switch statements, using popcnt" and "These numbers will change in a later patch, (when sparse maps make switch much more compact) so we shouldn't argue too much about this cut-off".

This makes the patch very hard to review. At least for me, it's not clear what you're trying to do.

Making the switch-to-lookup table transformation better is excellent, but it needs to be done by well-argued changes in clear and focused patches.


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

https://reviews.llvm.org/D60982