[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:412
+
+  Remark << ";";
 }
----------------
wenlei wrote:
> modimo wrote:
> > wenlei wrote:
> > > nit: any special reason for adding this? doesn't seem consistent with other remarks we have.
> > If you grab the remark outputs via `-Rpass=inline` you'll get additional suffix information:
> > ```
> > inline.cpp:8:12: remark: _Z3foov inlined into main with (cost=0, threshold=375) at callsite main:2:12; [-Rpass=inline]
> >     return foo();
> > ```
> > 
> > The semicolon is to separate the remark from any additional output at the end so when replaying we can match the correct callsite. Something like this would be unneeded for yaml replay but for the current implementation it's necessary for correctness.
> > 
> By correctness, did you mean the fact that you rely on `split(";")` in parsing, or something else?
> 
> This is not a big deal, but if no other remarks end with `;`, it would be good to be consistent. Using `split(";")` for parsing is just one way of implementing it, and IMO could be changed to favor consistency in remarks output.
> By correctness, did you mean the fact that you rely on `split(";")` in parsing, or something else?

Yeah, without that we would store the callsite from remarks as `main:2:12 [-Rpass=inline]` which would not match the actual callsite string `main:2:12` that we query the map with which causes replay to never inline.

> This is not a big deal, but if no other remarks end with `;`, it would be good to be consistent. Using `split(";")` for parsing is just one way of implementing it, and IMO could be changed to favor consistency in remarks output.

Doing a search query for `OptimizationRemarkAnalysis` I see vectorizer ORE uses "." for their terminator so switching to that is better consistency. I'll make the change in an upcoming patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94333