[llvm-commits] PATCH: Make instsimplify survive degenerate, unreachable code formations

[Duncan Sands]

Hi Chandler,

>     The current instsimplify ReplaceAndSimplifyAllUses function has a bug that
>     impacts its usage from inside the inliner, or in other contexts where it may
>     try to simplify code inside of an unreachable block. It checks initially
>     that From != To, but it then uses a WeakVH to keep track of both From and
>     To. The result is that mutually recursive values can, in some very strange
>     situations, end up replacing the From instruction with the To instruction
>     deep inside of a recursion. When popping back out, the WeakVH tracks this,
>     and we end up in an infinite loop trying to simplify an instruction with >0
>     uses into itself.

> --- a/include/llvm/Analysis/InstructionSimplify.h
> +++ b/include/llvm/Analysis/InstructionSimplify.h
> @@ -189,16 +189,29 @@ namespace llvm {
>                               const DominatorTree *DT = 0);
>
>
> -  /// ReplaceAndSimplifyAllUses - Perform From->replaceAllUsesWith(To) and then
> -  /// delete the From instruction.  In addition to a basic RAUW, this does a
> -  /// recursive simplification of the updated instructions.  This catches
> -  /// things where one simplification exposes other opportunities.  This only
> -  /// simplifies and deletes scalar operations, it does not change the CFG.
> +  /// \brief Replace all uses of 'I' with 'SimpleV' and simplify the uses
> +  /// recursively.
>    ///
> -  void ReplaceAndSimplifyAllUses(Instruction *From, Value *To,
> -                                 const TargetData *TD = 0,
> -                                 const TargetLibraryInfo *TLI = 0,
> -                                 const DominatorTree *DT = 0);
> +  /// This first performs a normal RAUW of I with SimpleV. It then recursively
> +  /// attempts to simplify those users updated by the operation. The 'I'
> +  /// instruction must not be equal to the simplified value 'SimpleV'.
> +  ///
> +  /// The function returns true if any recursive simplifications were
> +  /// performed.

maybe: The function returns true if any simplifications were performed.

> +  bool replaceAndRecursivelySimplify(Instruction *I, Value *SimpleV,
> +                                     const TargetData *TD = 0,
> +                                     const TargetLibraryInfo *TLI = 0,
> +                                     const DominatorTree *DT = 0);
> +
> +  /// \brief Recursively attempt to simplify an instruction.
> +  ///
> +  /// This routine uses SimplifyInstruction to simplify 'I', and if successful
> +  /// replaces uses of 'I' with the simplified value. It then recurses on each
> +  /// of the users impacted. It returns true iff 'I' simplified successfully.

While it is logically equivalent, I think it would be better to say:
It returns true if any simplifications were performed.


> +/// This routine returns 'true' only when *it* simplifies something. The passed
> +/// in simplified value does not count toward this.
> +static bool replaceAndRecursivelySimplifyImpl(Instruction *I, Value *SimpleV,
> +                                              const TargetData *TD,
> +                                              const TargetLibraryInfo *TLI,
> +                                              const DominatorTree *DT) {
> +  bool Simplified = false;
> +  SmallVector<Instruction *, 8> Worklist;
> +
> +  // If we have an explicit value to collapse to, do that round of the
> +  // simplification loop by hand initially.
> +  if (SimpleV) {
> +    for (Value::use_iterator UI = I->use_begin(), UE = I->use_end(); UI != UE;
> +         ++UI)
> +      Worklist.push_back(cast<Instruction>(*UI));
> +
> +    // Replace the instruction with its simpler

^ The end of the comment seems to be missing.

> +    I->replaceAllUsesWith(SimpleV);
> +    I->eraseFromParent();

Why assume that I has a parent here?

> +  } else {
> +    Worklist.push_back(I);
> +  }
>
> -    // Recursively simplify this user to the new value.
> -    ReplaceAndSimplifyAllUses(User, SimplifiedVal, TD, TLI, DT);
> -    From = dyn_cast_or_null<Instruction>((Value*)FromHandle);
> -    To = ToHandle;
> +  while (!Worklist.empty()) {
> +    I = Worklist.pop_back_val();
>
> -    assert(ToHandle && "To value deleted by recursive simplification?");
> +    // See if this instruction can simplify.

can simplify -> simplifies

> +    SimpleV = SimplifyInstruction(I, TD, TLI, DT);
> +    if (!SimpleV)
> +      continue;
> +
> +    Simplified = true;
> +
> +    // Stash away all the uses of the old instruction so we can check them for
> +    // recursive simplifications after a RAUW. This is cheaper than checking all
> +    // uses of To on the recursive step in most cases.
> +    for (Value::use_iterator UI = I->use_begin(), UE = I->use_end(); UI != UE;
> +         ++UI)
> +      Worklist.push_back(cast<Instruction>(*UI));
>
> -    // If the recursive simplification ended up revisiting and deleting
> -    // 'From' then we're done.
> -    if (From == 0)
> -      return;
> +    // Replace the instruction with its simpler

^ Unterminated comment.

> +    I->replaceAllUsesWith(SimpleV);
> +
> +    // Gracefully handle edge cases where the instruction is not wired into any
> +    // parent block.
> +    if (I->getParent())
> +      I->eraseFromParent();
>    }
> +  return Simplified;
> +}

Otherwise LGTM.

Ciao, Duncan.