<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jan 19, 2018 at 5:11 PM Matthias Braun via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">MatzeB accepted this revision.<br>
MatzeB added a comment.<br>
<br>
LGTM, I'm all for getting this into the LLVM tree, we can fine tune later as necessary.<br>
<br>
This is the first widely used machine module pass. Adding a module pass into the pipeline means we will have all the MI in memory at the same time rather than creating MI for 1 function at a time and freeing it after emitting each function. So it would be good to keep an eye on the compilers memory consumption... Maybe we can find a way to refactor the codegen pipeline later so that we can go back to 1 function at a time and still have a way to emit some new code afterwards...<br>
<br>
<br></blockquote><div><br></div><div>FWIW it would possible by splitting it into an IR pass to create the retpoline IR function if it notices a retpoline attributed function and a later MI pass to fill them in. I didn't do this originally, but it should be OK(tm).</div><div><br></div><div>-eric</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:412-413<br>
<br>
+  /// This pass may be implemented by targets that want to run passes<br>
+  /// that emit MI directly and bypass all other machine passes.<br>
+  virtual void addEmitPass() {}<br>
----------------<br>
Even though this documentation is mostly copied, how about changing it to:<br>
```<br>
Targets may add passes immediately before machine code is emitted in this callback.<br>
This is called later than addPreEmitPass().<br>
```<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:414<br>
+  /// that emit MI directly and bypass all other machine passes.<br>
+  virtual void addEmitPass() {}<br>
+<br>
----------------<br>
I find the name `addEmitPass()` misleading as it isn't adding the assembly emission passes. The best bad name I can think of right now is `addPreEmit2()`, in the spirit of the existing `addPreSched2()` callback...<br>
<br>
<br>
================<br>
Comment at: llvm/lib/Target/X86/X86RetpolineThunks.cpp:83-84<br>
+  auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();<br>
+  if (!TPC)<br>
+    return false;<br>
+<br>
----------------<br>
There shouldn't be any way to use any Target/X86 without also having a TargetPassConfig in the pipeline. An assert should be enough.<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D41723" rel="noreferrer" target="_blank">https://reviews.llvm.org/D41723</a><br>
<br>
<br>
<br>
</blockquote></div></div>