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

[Frits van Bommel]

On Sun, Dec 5, 2010 at 7:30 PM, Chris Lattner <clattner at apple.com> wrote:
>
> 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 :)

I agree I should probably have sent the simplifycfg and jump threading
patches separately.

The jump threading patches kind of go together though: the refactoring
makes adding the new functionality a much cleaner patch.

>> 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".

Sorry about that. I really try to do that, but it must be a muscle
memory thing or something :).

>  // 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".

That change accidentally got lost and ended up in patch #3.

> +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));

Not exactly, since Value isn't a Constant*, but I should have used
getKnownConstant(Value) here.

>       // 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>"

Good point.