[PATCH] D45657: [Instruction, BasicBlock] Add is_none_of and skipInsts helper functions.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added subscribers: dblaikie, timshen, debug-info.
vsk requested changes to this revision.
vsk added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:853
   const DataLayout &DL = BB->getParent()->getParent()->getDataLayout();
-  for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I) {
-    if (isa<DbgInfoIntrinsic>(I))
-      continue;
-
-    switch (I->getOpcode()) {
-    case Instruction::BitCast:
-    case Instruction::PtrToInt:
-    case Instruction::IntToPtr:
-    case Instruction::Alloca:
-      continue;
-    case Instruction::GetElementPtr:
-      if (cast<GetElementPtrInst>(I)->hasAllZeroIndices())
+  for (Instruction &I : BB->skipInsts<AllocaInst, BitCastInst, DbgInfoIntrinsic,
+                                      IntToPtrInst, PtrToIntInst>()) {
----------------
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.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:854
+  for (Instruction &I : BB->skipInsts<AllocaInst, BitCastInst, DbgInfoIntrinsic,
+                                      IntToPtrInst, PtrToIntInst>()) {
+    if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(&I))
----------------
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.


https://reviews.llvm.org/D45657