[llvm-commits] [Review request] [PR8714] Jump threading of indirectbr blocks (and more)

[Chris Lattner]

On Dec 5, 2010, at 12:24 PM, Frits van Bommel wrote:

> On Sun, Dec 5, 2010 at 7:30 PM, Chris Lattner <clattner at apple.com> wrote:
>> Otherwise, looks great, please commit and resend the followon patch, thanks!
> 
> An updated version of the follow-on patch is attached.
> 
> The description again:
> Implement jump threading of 'indirectbr' by keeping track of whether
> we're looking for ConstantInt*s or BlockAddress*s.
> 
> I wasn't sure what "bonus" to give for threading over an indirectbr. I
> figured a bit more than the one for threading over a switch, but I
> could be completely off base here. Anyone?

I think it should be a bit higher than switch, but not to much higher.  indbr is actually cheaper than a switch in execution and code size cost.  OTOH, indbr is much more rare, so bumping up the threshold (above that of a switch) is completely fine, because code growth is acceptable if we can get performance in cases that go through the trouble of using indbr.

Here are some comments:

-static Constant *getKnownConstant(Value *Val) {
+static Constant *getKnownConstant(Value* Val, bool wantBlockAddress) {

We've got * movement :)

Also, please capitalize wantBlockAddress for consistency.  Perhaps an enum would be better than a bool for the argument?  That would read better than the false in:

+      ComputeValueKnownInPredecessors(BO->getOperand(0), BB, LHSVals, false);


 // Helper method for ComputeValueKnownInPredecessors.  If Value is a
+// ConstantInt/BlockAddress (depending on which we want) or undef, push it.
+// Otherwise, do nothing.
 static void PushKnownConstantOrUndef(PredValueInfo &Result, Constant *Value,

It's somewhat independent from this patch, but (as a result of your previous changes) PushKnownConstantOrUndef is simple enough that it should just be inlined into its call sites: the resulting code would be more clear than with it out of line.

Otherwise, the patch and testcase look great.  Please commit with appropriate tweaks, and resolve the bugzilla.  Thanks Frits!

-Chris