[PATCH] D113839: [NFC] Use Optional<ProfileCount> to model invalid counts

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Generally like this sort of refactoring - though the const members are probably a fairly strong objection from me. The option to go further and make this a struct with optional helper member or non-member functions is less strenuous.



================
Comment at: llvm/include/llvm/IR/Function.h:257-258
   private:
-    uint64_t Count;
-    ProfileCountType PCT;
-    static ProfileCount Invalid;
+    const uint64_t Count;
+    const ProfileCountType PCT;
 
----------------
LLVM code doesn't generally use top-level const. Especially for members this makes things difficult - for instance you can't copy-assign objects with const members (so `ProfileCount a, b; a = b;` would be invalid) which usually isn't intended & can make things harder to work with.


================
Comment at: llvm/include/llvm/IR/Function.h:261-265
     ProfileCount(uint64_t Count, ProfileCountType PCT)
         : Count(Count), PCT(PCT) {}
-    bool hasValue() const { return PCT != PCT_Invalid; }
     uint64_t getCount() const { return Count; }
     ProfileCountType getType() const { return PCT; }
     bool isSynthetic() const { return PCT == PCT_Synthetic; }
----------------
Could this just be a struct at this point, maybe with an "isSynthetic" free function if testing "type == Synthetic" is too verbose?

The members are all both directly accessible, and can be initialized to any value, so the encapsulation doesn't seem to be providing anything?


================
Comment at: llvm/lib/IR/Function.cpp:1927
         return ProfileCount(Count, PCT_Real);
       } else if (AllowSynthetic &&
                  MDS->getString().equals("synthetic_function_entry_count")) {
----------------
separate patch could remove this else-after-return (& maybe add {} to the top-level `if` - it's a bit long to have no braces, even if it is only a single statement - or alternativeyl, invert that if statement and return early (`if (!MD || !MD->getOperand(0)) return None;` - oh, also could skip the getOperand checking and use dyn_cast_or_null on the next line instead))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113839