[PATCH] D157358: [FunctionPropertiesAnalysis] Add detailed analysis

[Aiden Grossman via Phabricator via llvm-commits]

aidengrossman added inline comments.


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:90
+
+    unsigned PredecessorCount = succ_size(&BB);
+    if (PredecessorCount == 1)
----------------
mtrofin wrote:
> `pred_size`
> 
> Is the test catching this?
Tests were not currently catching that. I've added a new unit test case to make sure the behavior is as expected when the number of successors and predecessors is different.


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:105
+
+    for (const Instruction &I : BB) {
+      if (I.isCast())
----------------
mtrofin wrote:
> probably want to ignore debug instructions
> 
> also, do you care if it's an intrinsic?
Not currently. I'm probably going to add a couple more features in the future and a count of instructions that are intrinsics would be nice to have, but currently focused on getting the original feature set in.


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:119
+          if (C->getType()->isIntegerTy())
+            IntegerConstantCount += 1;
+          else if (C->getType()->isFloatTy())
----------------
mtrofin wrote:
> `++IntegerConstantCount`?
I've switched to doing `+= Direction` so that this better matches the behavior of the rest of the function when updating a function analysis. It doesn't really matter in my case, but definitely shouldn't purposefully be different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157358