[PATCH] D141393: [llvm][ir] Purge MD_prof custom accessors

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

great cleanup overall.



================
Comment at: llvm/lib/IR/ProfDataUtils.cpp:46
 // the minimum number of operands for MD_prof nodes with branch weights
-constexpr unsigned MinBWOps = 3;
+constexpr unsigned MinBWOps = 2;
 
----------------
paulkirth wrote:
> This constant is used to check the validity of branch weight metadata. Changing this to 2  implies that a MD_prof node w/ only the "branch_weight" moniker and a single weight are valid. I don't think that's true, so I don't think this should change unless we change how MD_prof is laid out as a whole.
agree. What the reason to change it to 2?


================
Comment at: llvm/lib/IR/ProfDataUtils.cpp:128
+          I.getOpcode() == Instruction::Select ||
+          I.getOpcode() == Instruction::Switch) &&
+         "Looking for branch weights on something besides branch, select, or "
----------------
paulkirth wrote:
> This is wrong. you can't include a switch here. The overload above takes a `SmallVectorImpl` is the only appropriate API when dealing with a switch, since it can have an arbitrary number of arms.
> 
> The assertion here is to prevent API misuse. If used on a switch, this will always return false, so I don't think thats the right choice here.  You get some utility by being able to use this in  more places, but I think this will be easy to introduce subtle bugs. For instance, if a switch happens to only have 2 arms this will work when it arguably shouldn't. 
> 
> My preference would be that everything settle on the `SmallVectorImpl` API, since that can correctly handle all cases. It isn't' as ergonomic in some of the code uses though, IIRC.
Agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141393