[PATCH] D99169: [DebugInfo] Replace debug uses in replaceUsesOutsideBlock

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando updated this revision to Diff 336089.
Orlando marked an inline comment as done.
Orlando added a comment.

+ Remove tbaa metadata from lit test.
+ Fix clang-tidy warning in unittest.
+ Move the code into a new function `replaceDbgUsesOutsideBlock`.

Hi @djtodoro, is this what you meant?

Sorry for the delay, I was investigating what we could do with `replaceUsesWithIf` as an alternative. `replaceUsesWithIf` takes a predicate function which has a `Use` parameter. Looking at current `replaceUsesWithIf` call sites I noticed that most of the predicates actually only look at the `Use`'s `User` rather than the `Use` itself. I came up with an overload (code below) which accepts a predicate with a `User` parameter rather than `Use`. This allows s to use that single predicate for both normal uses and llvm.dbg.* (metadata-as-value value-as-metadata) uses.

Existing `replaceUsesWithIf` declaration in Value.h:

  void replaceUsesWithIf(Value *New, llvm::function_ref<bool(Use &U)> ShouldReplace)

New `replaceUsesWithIf` overload:

  void Value::replaceUsesWithIf(
      Value *New, llvm::function_ref<bool(User *U)> ShouldReplaceIn) {
    assert(New && "Value::replaceUsesWithIf(<null>) is invalid!");
    assert(New->getType() == getType() &&
           "replaceUses of value with new value of different type!");
    SmallVector<User *> Users(users());
    for (auto *U : Users) {
      if (ShouldReplaceIn(U))
        U->replaceUsesOfWith(this, New);
    }
    SmallVector<DbgVariableIntrinsic *> DbgUsers;
    findDbgUsers(DbgUsers, this);
    for (auto *DVI : DbgUsers) {
      if (ShouldReplaceIn(DVI))
        DVI->replaceVariableLocationOp(this, New);
    }
  }

Existing `replaceUsesWithIf` call sites can be updated to use the new `User` predicate, e.g. the change to this patch would be:

  @@ -564,8 +555,9 @@ void Value::replaceUsesOutsideBlock(Value *New, BasicBlock *BB) {
            "replaceUses of value with new value of different type!");
     assert(BB && "Basic block that may contain a use of 'New' must be defined\n");
   
  +  replaceUsesWithIf(New, [BB](User *U) {
  +      auto *I = dyn_cast<Instruction>(U);
  -  replaceDbgUsesOutsideBlock(this, New, BB);
  -  replaceUsesWithIf(New, [BB](Use &U) {
  -    auto *I = dyn_cast<Instruction>(U.getUser());
       // Don't replace if it's an instruction in the BB basic block.
       return !I || I->getParent() != BB;
     });

The benefit of this is that most `replaceUsesWithIf` calls can handle llvm.dbg.* updates with a minimal change. The downsides are that it touches more code, and creates a slightly confusing interface where one `replaceUsesWithIf` overload updates debug uses and the other does not. So, maybe it's better to keep the fix simple (the patch as it exists in review now) - I'm unsure. wdyt?


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

https://reviews.llvm.org/D99169

Files:
  llvm/lib/IR/Value.cpp
  llvm/test/DebugInfo/Generic/instcombine-replaced-select-with-operand.ll
  llvm/unittests/IR/ValueTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D99169.336089.patch
Type: text/x-patch
Size: 11613 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210408/4cdf2746/attachment-0001.bin>