On 2014-Apr-15, at 23:28, Chandler Carruth <[email protected]> wrote:
> I don't really understand the use of RAV here,
I think Bob's idea in r206039 is to avoid repeating the traversal logic.
I like the direction myself.
> or the logic of having a secondary switch, but at this point, there are too
> many patches flying and I feel like there is some more extensive discussion
> that took place somewhere other than this thread. =/
Not really. Just coordination around how to stage changes we knew would
conflict. In the end we reversed them.
> No fun when you get bounced back and forth in code review.
I can take it :).
> I'm happy to go with the implementation strategy as you have it. I may even
> write up a patch to show how I'm imagining you might simplify this. Anyways...
>
> Some more microscopic nits. Feel free to commit whenever.
r206397
> I like where this design has ended up, and thanks for seeing it through. =]
Thanks for your help designing the hash -- much better than where this
started off!
> +/// \brief Stable hasher for PGO region counters.
> +///
> +/// PGOHash produces a stable hash of a given function's control flow.
> +///
> +/// Changing the output of this hash will invalidate all previously generated
> +/// profiles -- i.e., don't do it.
> +///
> +/// \note When this hash does eventually change (years?), we still need to
> +/// support old hashes. We'll need to pull in the version number from the
> +/// profile data format and use the matching hash function.
> +class PGOHash {
> + constexpr static int NumBitsPerType = 6;
> + constexpr static unsigned NumTypesPerWord =
> + sizeof(uint64_t) * 8 / NumBitsPerType;
> + constexpr static unsigned TooBig = 1u << NumBitsPerType;
>
> I would sink these to live down with the static_assert, and sink all of it
> into the existing private section with member variables.
Yup, better to merge the private sections here.
Also, turns out MSVC doesn't like constexpr; I switched to static const.
> +
> +public:
> + /// \brief Hash values for AST nodes.
> + ///
> + /// Distinct values for AST nodes that have region counters attached.
> + ///
> + /// These values must be stable. All new members must be added at the end,
> + /// and no members should be removed. Changing the enumeration value for
> an
> + /// AST node will affect the hash of every function that contains that
> node.
> + enum HashType : unsigned char {
> + None = 0,
> + LabelStmt = 1,
> + WhileStmt,
> + DoStmt,
> + ForStmt,
> + CXXForRangeStmt,
> + ObjCForCollectionStmt,
> + SwitchStmt,
> + CaseStmt,
> + DefaultStmt,
> + IfStmt,
> + CXXTryStmt,
> + CXXCatchStmt,
> + ConditionalOperator,
> + BinaryOperatorLAnd,
> + BinaryOperatorLOr,
> + BinaryConditionalOperator,
> +
> + // Keep this last. It's for the static assert that follows.
> + LastHashType
> + };
> + static_assert(LastHashType <= TooBig, "Too many types in HashType");
> +
> +private:
> + uint64_t Working;
> + unsigned Count;
> + llvm::MD5 MD5;
> +
> +public:
> + // TODO: When this format changes, take in a version number here, and use
> the
> + // old hash calculation for file formats that used the old hash.
> + PGOHash() : Working(0), Count(0) {}
> + void combine(HashType Type);
> + uint64_t finalize();
>
> I would just put these up with the public enum. Excepting private types or
> enums which are necessary to define prior to the public interface (because it
> is defined in terms of them), I generally prefer the public interface at the
> top followed by the private interface. Mild preference though.
My preference is to put member variables before functions, but I
shuffled things a little to make it cleaner.
> +};
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
