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

Fri Aug 25 12:02:06 PDT 2023

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

================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:294
+
+  uint32_t ExitWeight0; // aka x0
+  if (!ConditionalPreHeader) {
----------------
mtrofin wrote:
> MatzeB wrote:
> > mtrofin wrote:
> > > nit: can you init ExitWeight0 at declaration (to avoid potential joys due to uninitialization later). In fact, setting it to 0 and flipping the conditional below and not having an else would do it.
> > This is deliberately left uninitialized! The idea is that every branch in the following ifs will set a value for it. Leaving the variable uninitialized here, means we will get a compiler warning if we forget to set the value on any branch below which I think is more valuable than setting some kind of safe default here but not getting a warning anymore.
> Is it safe to rely on warnings though - is there at least a bot that treats uninitialized values as errors? But that aside, looks like 0 is the value when `ConditionalPreHeader` anyway, meaning 0 wouldn't be some safe default, rather, it's exactly what you'd want (assuming the if condition were swapped)
Not sure if we have a `-Werror` bot, but the uninitialized variable warnings has always been part of `-Wall`. Also LLVM is warning free for most people (certainly is for me), new warning attract attention immediately. I have seen multiple instances in the past where someone added a new case to a complicated if-cascade but forgot to set a variables where the default was not suitable...

Probably not worth getting hung up on this particular one, so I'll change the default to `= 0` and drop the `ConditionalPreHeader` case. But for the record: I disagree and certainly will not change other places in LLVM where I used similar patterns :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157462