[PATCH] D62122: [NFC] Introduce SwitchInst wrapper for prof branch_weights handling

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/IR/Instructions.h:3440
+/// their prof branch_weights metadata.
+class SwitchInstProfBranchWeightsWrapper {
+  SwitchInst &SI;
----------------
Nit: Perhaps name it SwitchInstProfUpdateWrapper ?   The original name implies that SwitchInst does not have prof weights which is misleading.


================
Comment at: llvm/include/llvm/IR/Instructions.h:3455
+  SwitchInst *operator->() { return &SI; }
+  SwitchInst &operator*() { return SI; }
+  operator SwitchInst *() { return &SI; }
----------------
nit: is it necessary to have 3 things doing the same thing?


================
Comment at: llvm/include/llvm/IR/Instructions.h:3466
+
+  SwitchInst::CaseIt removeCase(SwitchInst::CaseIt I);
+
----------------
add documentation comment.


================
Comment at: llvm/include/llvm/IR/Instructions.h:3468
+
+  void addCase(ConstantInt *OnVal, BasicBlock *Dest, CaseWeightOpt W);
+
----------------
same here.


================
Comment at: llvm/lib/IR/Instructions.cpp:3891
+
+  if (AllZeroes || Weights.getValue().size() < 2)
+    return nullptr;
----------------
Add sanity check that number of cases matches weights size?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62122