[PATCH] D139337: [IRSim] Treat Branch OperVals different from regular operands

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:102
+  for (Value *V : getBlockOperVals()) {
+    BasicBlock *Successor = dyn_cast<BasicBlock>(V);
+    assert (Successor && "Not a basic block");
----------------
This can use `cast<>`. Then you get an assertion on the wrong type for free.

https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:115
+ArrayRef<Value *> IRInstructionData::getBlockOperVals() {
+  assert(isa<BranchInst>(Inst) ||
+         isa<PHINode>(Inst) && "Instruction must be branch or PHINode");
----------------
I think this needs brackets around the `isa`s otherwise the `&&` will be evaluated first.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:725
   // Get the basic blocks the label refers to.
-  BasicBlock *ABB = static_cast<BasicBlock *>(A.OperVal);
-  BasicBlock *BBB = static_cast<BasicBlock *>(B.OperVal);
+  BasicBlock *ABB = dyn_cast<BasicBlock>(A.OperVal);
+  BasicBlock *BBB = dyn_cast<BasicBlock>(B.OperVal);
----------------
I think you can use `cast<>` for both of these.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:856
     if (RelBlockLocsA.size() != RelBlockLocsB.size() &&
-        OperValsA.size() != OperValsB.size())
+        ABL.size() != BBL.size())
       return false;
----------------
could use a comment?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:859
 
+    assert(RelBlockLocsA.size() == ABL.size() &&
+           "Block information vectors not the same size.");
----------------
Why are these asserts after the `if`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139337