[PATCH] D63378: [ORC] WIP Speculative compilation
Lang Hames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 31 10:44:28 PDT 2019
lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.
Minor style nits aside, this looks great. Once you've addressed them (and run clang-format over the patch) feel free to commit this to the mainline and continue development there. :)
================
Comment at: llvm/examples/SpeculativeJIT/SpeculativeJIT.cpp:198
+}
\ No newline at end of file
----------------
There should be a newline here.
================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h:23
+#include "llvm/Support/FileOutputBuffer.h"
namespace llvm {
----------------
Is this FileOutputBuffer include needed?
================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h:188
IndirectStubsManager &ISManager, JITDylib &SourceJD,
- SymbolAliasMap CallableAliases, VModuleKey K = VModuleKey()) {
+ SymbolAliasMap CallableAliases, ImplSymbolMap *SrcJDLoc,
+ VModuleKey K = VModuleKey()) {
----------------
The SrcJDLoc parameter should have a default value of nullptr, so that people who are not using speculation can ignore it.
================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:54
+ else
+ return {};
+ }
----------------
It's best to use "return None" rather than "return {}". I've seen some older compilers struggle with the latter.
================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:71
+ SymbolNameSet likelySymbols) {
+ std::lock_guard<std::mutex> lockit(ConcurrentAccess);
+ GlobalSpecMap.insert({ImplAddr, std::move(likelySymbols)});
----------------
The LLVM style guide says that variable names should start with a capital letter, so lockit should be Lockit.
================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:80
+ {
+ std::lock_guard<std::mutex> lockit(ConcurrentAccess);
+ auto It = GlobalSpecMap.find(FAddr);
----------------
Lockit should start with a capital letter here too.
================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:114
+ Speculator &operator=(Speculator &&) = delete;
+ virtual ~Speculator() {}
+
----------------
Speculator doesn't seem to have any virtual methods. Are you intending for this to be subclassed? If not, you can remove the virtual destructor too.
================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:163-170
+ template <
+ typename AnalysisTy,
+ typename std::enable_if<
+ std::is_base_of<AnalysisInfoMixin<AnalysisTy>, AnalysisTy>::value,
+ bool>::type = true>
+ void registerAnalysis() {
+ FAM.registerPass([]() { return AnalysisTy(); });
----------------
I think this should be able to be removed now?
================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:76
+ // ignoring number of direct calls in a BB
+ auto topk = numBBToGet(BBFreqs.size());
+
----------------
topk should start with a capital letter.
================
Comment at: llvm/lib/ExecutionEngine/Orc/Speculation.cpp:30
+ assert(SrcJD && "Tracking on Null Source .impl dylib");
+ std::lock_guard<std::mutex> lockit(ConcurrentAccess);
+ for (auto &I : ImplMaps) {
----------------
lockit should start with a capital letter.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63378/new/
https://reviews.llvm.org/D63378
More information about the llvm-commits
mailing list