[PATCH] D157462: LoopRotate: Add code to update branch weights

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:291-306
+  uint32_t ExitCount0; // aka x0
+  if (!ConditionalPreHeader) {
+    // pre-header turned into an unconditional jump.
+    ExitCount0 = 0;
+  } else {
+    // Here we cannot know how many 0-trip count loops we have, so we guess:
+    if (Weights[1] > Weights[0]) {
----------------
MatzeB wrote:
> MatzeB wrote:
> > paulkirth wrote:
> > > This could probably be a helper function. It would also allow for expressive names for the `Weight[0]`/`Weight[1]` values, which will probably make it easier to follow. 
> > > 
> > > Alternatively, you could group all of these counts into a struct. I'm not convinced that is worth it in this case, but it may make the code simpler to reason about and keep all the logic regarding the heuristic together.
> > > 
> > > This does come down to personal coding style, so don't consider these suggestions blocking.
> > What's the point though? The only other thing here is the construction of two MDNodes. Feels needlessly complicated to introduce a struct and separate function just for that...
> I did introduce two local variables now, that give a name to `Weights[0]` and `Weights[1]`, hope that is enough to resolve this.
Sorry, I could have worded that better as a "just a suggestion you can feel free to ignore".  I tried to convey that at the end, but clearly missed my mark.

I was fine with the previous version, but those two thoughts came to mind, so I mentioned them. So nothing to resolve :)

> What's the point though? The only other thing here is the construction of two MDNodes. Feels needlessly complicated to introduce a struct and separate function just for that...

Fair points. My thoughts were more about making this function easier to understand, but you're right that it probably isn't necessary and isn't that hard to follow right now. 

Please feel free to ignore those suggestions, or restore the previous version if you'd rather.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157462