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

[Chris Lattner]

On Dec 2, 2010, at 9:10 AM, Frits van Bommel wrote:

> The first four attached patches implement jump threading of
> 'indirectbr' instructions when the address being jumped to is a known
> blockaddress in a predecessor (or just a constant block address), in
> these stages:

Thanks for working on this!  It's generally best to send in individual patches instead of series, but this is nice work :)

> 0) Remove trailing whitespace.

ok.

> 1) Refactor jump threading. Should have no functional change other
> than the order of two transformations that are mutually-exclusive[1]
> and the exact formatting of debug output. Internally, it now stores
> the ConstantInt*s as Constant*s, and actual undef values instead of
> nulls. Splitting this out hopefully makes the actual patch much
> simpler to review.

This is a great improvement, thanks!  Some minor comments:

+static Constant *getKnownConstant(Value* Val) {
+static void PushKnownConstantOrUndef(PredValueInfo &Result, Constant *Value,
+                                     BasicBlock* BB) {

Please watch * placement, we prefer "Value *Val".

 // Helper method for ComputeValueKnownInPredecessors.  If Value is a
 // ConstantInt, push it.  If it's an undef, push 0.  Otherwise, do nothing.

Please update this comment now that it doesn't "push 0".

+static void PushKnownConstantOrUndef(PredValueInfo &Result, Constant *Value,
+                                     BasicBlock* BB) {
   if (ConstantInt *FoldedCInt = dyn_cast<ConstantInt>(Value))
     Result.push_back(std::make_pair(FoldedCInt, BB));
+  else if (UndefValue *Undef = dyn_cast<UndefValue>(Value))
+    Result.push_back(std::make_pair(Undef, BB));
 }

You could simplify this function to:

  if (isa<ConstantInt>(Value) || isa<UndefValue>(Value))
    Result.push_back(std::make_pair(Value, BB));


       // Invert the known values.
       for (unsigned i = 0, e = Result.size(); i != e; ++i)
-        if (Result[i].first)
+        if (!isa<UndefValue>(Result[i].first))
           Result[i].first =
             cast<ConstantInt>(ConstantExpr::getNot(Result[i].first));

You can remove the if and cast<ConstantInt> now, "not undef" = undef.  Before it was just avoid "not <null>"

Otherwise, looks great, please commit and resend the followon patch, thanks!

-Chris