[PATCH] D139346: [FuncSpec] Global ranking of specialisations

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:237-244
+hash_code llvm::hash_value(const ArgInfo &A) {
+  return hash_combine(hash_value(A.Formal), hash_value(A.Actual));
+}
+
+hash_code llvm::hash_value(const SpecSig &S) {
+  return hash_combine(hash_value(S.Key),
+                      hash_combine_range(S.Args.begin(), S.Args.end()));
----------------
labrinea wrote:
> Can we move these in the corresponding header files, where they are declared as friends? I am seeing a warning when I compile:
> > warning: ‘llvm::hash_code llvm::hash_value(const llvm::ArgInfo&)’ has not been declared within ‘llvm’
> > warning: ‘llvm::hash_code llvm::hash_value(const llvm::SpecSig&)’ has not been declared within ‘llvm’
> 
> 
The friend declarations in the header file refer to names in the innermost enclosing namespace (https://eel.is/c++draft/dcl.meaning#general-2.3 ) , which is `llvm` in this case. GCC seems a bit overzealous here, but
I don't mind moving these functions to the header, in spirit they are quite like `operator==`, etc.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:246-258
+template <> struct llvm::DenseMapInfo<SpecSig> {
+  static inline SpecSig getEmptyKey() { return {~0U, {}}; }
+
+  static inline SpecSig getTombstoneKey() { return {~1U, {}}; }
+
+  static unsigned getHashValue(const SpecSig &S) {
+    return static_cast<unsigned>(hash_value(S));
----------------
labrinea wrote:
> Should we maybe move this one too inside `FunctionSpecialization.h` ?
What would we gain from it?

This is purely an implementation detail that is best kept away from header files.


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

https://reviews.llvm.org/D139346