[PATCH] D135411: Add generic KCFI operand bundle lowering

[Sami Tolvanen via Phabricator via llvm-commits]

samitolvanen added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/KCFI.cpp:48-49
+  SmallVector<CallInst *, 8> KCFICalls;
+  for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; ++I) {
+    if (auto *CI = dyn_cast<CallInst>(&*I))
+      if (CI->getOperandBundle(LLVMContext::OB_kcfi))
----------------
nickdesaulniers wrote:
> prefer a range-for here
> ```
> for (Instruction &I : F)
>   if (auto &CI = dyn_cast<CallInst>(I);
> ```
`Function` doesn't directly support iterating through instructions. I used `inst_iterator` here to avoid looping through basic blocks as well. Your example results in:
```
error: non-const lvalue reference to type 'llvm::Instruction' cannot bind to a value of unrelated type 'llvm::BasicBlock'
```


================
Comment at: llvm/test/Transforms/KCFI/kcfi-supported.ll:4
+
+; COM: If the back-end supports KCFI operand bundle lowering, KCFIPass should be a no-op.
+
----------------
nickdesaulniers wrote:
> COM?
https://llvm.org/docs/CommandGuide/FileCheck.html#the-com-directive

But yes, not needed here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135411