[BasicBlock] Add instructionsWithoutDebug methods to skip debug insts.

[mattd]

include/llvm/IR/BasicBlock.h

185 ↗

(On Diff #142527)

I realize \brief is used throughout this header, but I do not think that is necessary anymore, because the Doxygen.cfg has AutoBrief on.

include/llvm/IR/Instruction.h

703 ↗

(On Diff #142527)

I like how you made this a struct instead of just a variadic function. I was exploring a similar solution, as discussed in the aforementioned thread, but had ambiguity issues with the base case. Looks like you cracked that! I like this.

I wonder if it makes more sense to push this up into Casting.h since it's as specific as isa<>.

[aprantl]

include/llvm/IR/BasicBlock.h

185 ↗

(On Diff #142527)

Yes, please avoid \brief.
Bonus points for a separate NFC commit that removes \brief from the entire header :-)

[vsk]

I like the direction this is going in :).

I think having an API like 'instructions_no_debug' or similar in BasicBlock would make the interface friendlier. For one, it'd signal to readers that skipping debug intrinsics is an established pattern. Also, I think it would be easier to change if, e.g we wanted update all existing users of the API to skip lifetime intrinsics too.

lib/Transforms/IPO/PartialInlining.cpp

853 ↗

(On Diff #142527)

I think this changes the inline cost of debug info intrinsics from InstrCost to 0. That seems like a correct and worthwhile change, but would probably be best to tackle in a follow-up patch.

854 ↗

(On Diff #142527)

Fwiw, I found the switch a little easier to read. I'm not sure it's a clear readability improvement to put these type checks into template parameters.

[dblaikie]

In D45657#1068834, @vsk wrote:

I like the direction this is going in :).

I think having an API like 'instructions_no_debug' or similar in BasicBlock would make the interface friendlier. For one, it'd signal to readers that skipping debug intrinsics is an established pattern. Also, I think it would be easier to change if, e.g we wanted update all existing users of the API to skip lifetime intrinsics too.

+1

include/llvm/IR/BasicBlock.h

198 ↗

(On Diff #142527)

Indirection through a function pointer may not be optimized away - may be worth baking this into a functor type?

[mattd]

include/llvm/IR/BasicBlock.h

185 ↗

(On Diff #142527)

Bonus points for a separate NFC commit that removes \brief from the entire header :-)

Challenge accepted.

[fhahn]

In D45657#1068834, @vsk wrote:

I like the direction this is going in :).

I think having an API like 'instructions_no_debug' or similar in BasicBlock would make the interface friendlier. For one, it'd signal to readers that skipping debug intrinsics is an established pattern. Also, I think it would be easier to change if, e.g we wanted update all existing users of the API to skip lifetime intrinsics too.

Thanks for the feedback! I'll add a instructionsNoDebug function. I tried to make skipInsts as flexible as possible, but I am not sure if the flexibility is really needed. Should I remove it for now?

include/llvm/IR/BasicBlock.h

198 ↗

(On Diff #142527)

Will do

unittests/IR/BasicBlockTest.cpp

17 ↗

(On Diff #142527)

I'll remove this.

[vsk]

In D45657#1070122, @fhahn wrote:

In D45657#1068834, @vsk wrote:

I like the direction this is going in :).

I think having an API like 'instructions_no_debug' or similar in BasicBlock would make the interface friendlier. For one, it'd signal to readers that skipping debug intrinsics is an established pattern. Also, I think it would be easier to change if, e.g we wanted update all existing users of the API to skip lifetime intrinsics too.

Thanks for the feedback! I'll add a instructionsNoDebug function. I tried to make skipInsts as flexible as possible, but I am not sure if the flexibility is really needed. Should I remove it for now?

It should be possible to implement instructionsNoDebug succinctly without skipInsts, so I'd suggest removing it.

I think the is_{any,none}_of predicates could be generally useful, though. It'd make sense to add them as a follow-up.

[fhahn]

Simplified things, only add methods to skip debug instructions.

[fhahn]

The only problem with those functions is that we cannot reverse a filter_iterator at the moment, as needed for D45379. But I suppose that should be fixable too.

[mattd]

Thanks! This looks really nice.

[vsk]

Thanks, lgtm as well. I sent out a patch to fix the issue with reversing filtered iterator ranges (https://reviews.llvm.org/D45792).

llvm/trunk/include/llvm/IR/BasicBlock.h

public:
   ///
   /// In particular, it skips all PHIs and LandingPad instructions.
   const_iterator getFirstInsertionPt() const;
   iterator getFirstInsertionPt() {
     return static_cast<const BasicBlock *>(this)
                                           ->getFirstInsertionPt().getNonConst();
   }
 
+  /// Return a const iterator range over the instructions in the block, skipping
+  /// any debug instructions.
+  iterator_range<filter_iterator<BasicBlock::const_iterator,
+                                 std::function<bool(const Instruction &)>>>
+  instructionsWithoutDebug() const;
+
+  /// Return an iterator range over the instructions in the block, skipping any
+  /// debug instructions.
+  iterator_range<filter_iterator<BasicBlock::iterator,
+                                 std::function<bool(Instruction &)>>>
+  instructionsWithoutDebug();
+
   /// Unlink 'this' from the containing function, but do not delete it.
   void removeFromParent();
 
   /// Unlink 'this' from the containing function and delete it.
   ///
   // \returns an iterator pointing to the element after the erased one.
   SymbolTableList<BasicBlock>::iterator eraseFromParent();
 

llvm/trunk/lib/IR/BasicBlock.cpp

BasicBlock::~BasicBlock() {
   InstList.clear();
 }
 
 void BasicBlock::setParent(Function *parent) {
   // Set Parent=parent, updating instruction symtab entries as appropriate.
   InstList.setSymTabObject(&Parent, parent);
 }
 
+iterator_range<filter_iterator<BasicBlock::const_iterator,
+                               std::function<bool(const Instruction &)>>>
+BasicBlock::instructionsWithoutDebug() const {
+  std::function<bool(const Instruction &)> Fn = [](const Instruction &I) {
+    return !isa<DbgInfoIntrinsic>(I);
+  };
+  return make_filter_range(*this, Fn);
+}
+
+iterator_range<filter_iterator<BasicBlock::iterator,
+                               std::function<bool(Instruction &)>>>
+BasicBlock::instructionsWithoutDebug() {
+  std::function<bool(Instruction &)> Fn = [](Instruction &I) {
+    return !isa<DbgInfoIntrinsic>(I);
+  };
+  return make_filter_range(*this, Fn);
+}
+
 void BasicBlock::removeFromParent() {
   getParent()->getBasicBlockList().remove(getIterator());
 }
 
 iplist<BasicBlock>::iterator BasicBlock::eraseFromParent() {
   return getParent()->getBasicBlockList().erase(getIterator());
 }
 

llvm/trunk/unittests/IR/BasicBlockTest.cpp

TEST(BasicBlockTest, PhiRange) {
   // And iterate a const range.
   for (const auto &PN : const_cast<const BasicBlock *>(BB.get())->phis()) {
     EXPECT_EQ(BB.get(), PN.getIncomingBlock(0));
     EXPECT_EQ(BB1.get(), PN.getIncomingBlock(1));
     EXPECT_EQ(BB2.get(), PN.getIncomingBlock(2));
   }
 }
 
+#define CHECK_ITERATORS(Range1, Range2)                                        \
+  EXPECT_EQ(std::distance(Range1.begin(), Range1.end()),                       \
+            std::distance(Range2.begin(), Range2.end()));                      \
+  for (auto Pair : zip(Range1, Range2))                                        \
+    EXPECT_EQ(&std::get<0>(Pair), std::get<1>(Pair));
+
+TEST(BasicBlockTest, TestSkipInsts) {
+  LLVMContext Ctx;
+
+  std::unique_ptr<Module> M(new Module("MyModule", Ctx));
+  Type *ArgTy1[] = {Type::getInt32PtrTy(Ctx)};
+  FunctionType *FT = FunctionType::get(Type::getVoidTy(Ctx), ArgTy1, false);
+  auto *V = new Argument(Type::getInt32Ty(Ctx));
+  Function *F = Function::Create(FT, Function::ExternalLinkage, "", M.get());
+
+  Value *DbgAddr = Intrinsic::getDeclaration(M.get(), Intrinsic::dbg_addr);
+  Value *DbgDeclare =
+      Intrinsic::getDeclaration(M.get(), Intrinsic::dbg_declare);
+  Value *DbgValue = Intrinsic::getDeclaration(M.get(), Intrinsic::dbg_value);
+  Value *DIV = MetadataAsValue::get(Ctx, (Metadata *)nullptr);
+  SmallVector<Value *, 3> Args = {DIV, DIV, DIV};
+
+  BasicBlock *BB1 = BasicBlock::Create(Ctx, "", F);
+  const BasicBlock *BBConst = BB1;
+  IRBuilder<> Builder1(BB1);
+
+  AllocaInst *Var = Builder1.CreateAlloca(Builder1.getInt8Ty());
+  Builder1.CreateCall(DbgValue, Args);
+  Instruction *AddInst = cast<Instruction>(Builder1.CreateAdd(V, V));
+  Instruction *MulInst = cast<Instruction>(Builder1.CreateMul(AddInst, V));
+  Builder1.CreateCall(DbgDeclare, Args);
+  Instruction *SubInst = cast<Instruction>(Builder1.CreateSub(MulInst, V));
+  Builder1.CreateCall(DbgAddr, Args);
+
+  SmallVector<Instruction *, 4> Exp = {Var, AddInst, MulInst, SubInst};
+  CHECK_ITERATORS(BB1->instructionsWithoutDebug(), Exp);
+  CHECK_ITERATORS(BBConst->instructionsWithoutDebug(), Exp);
+}
+
 } // End anonymous namespace.
 } // End llvm namespace.