[PATCH] D88363: [CodeGen] Improve likelihood attribute branch weights

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1622
     EmitBranchOnBoolExpr(CondOp->getCond(), LHSBlock, RHSBlock,
-                         getProfileCount(CondOp), Weights);
+                         getProfileCount(CondOp), Stmt::LH_None);
 
----------------
aaron.ballman wrote:
> Why `LH_None` in this case? (Also, given that it's the default value, should you skip passing this at all? Or should the parameter not have a default value, perhaps?)
This should be `LH_None` since this is the likelihood of `C ? T : F` and there's no likelihood for this expression. When used in `if(C ? T : F)` the if can have an likelihood attribute for the `if`. That's why the other two calls in this block have a likelihood. I'll add comment to make it clear why this is correct. I felt using the explicit value here instead of the defaulted value was clearer. This would be the only call in this function not to explicitly use a value.

Since `EmitBranchOnBoolExpr` is also used at other places I prefer to keep the default argument.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1678-1679
 
   // Create branch weights based on the number of times we get here and the
   // number of times the condition should be true.
+  llvm::MDNode *Weights = nullptr;
----------------
aaron.ballman wrote:
> This comment is now a bit stale.
Agreed, I'll remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88363