[PATCH] D74904: [CallSiteInfo] Handle bundles when updating call site info

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added a comment.

In D74904#1893729 <https://reviews.llvm.org/D74904#1893729>, @dstenb wrote:

> In D74904#1893374 <https://reviews.llvm.org/D74904#1893374>, @djtodoro wrote:
>
> > In D74904#1893241 <https://reviews.llvm.org/D74904#1893241>, @djtodoro wrote:
> >
> > > I see this was triggered in a case when we have not used the '-g' option at all.
> > >
> > > When we started the work, there were thoughts the "call-site-info" could be used somewhere else, but it is not the case now. It is being used only for the Debug Entry Values feature, so I think it makes sense to avoid production (and handling though out the pipeline) of the call-site-info if not '-g' and '-O>0'.
> > >
> > > The DWARF generation (for call-site-params) is controlled the same way as production of call-sites by looking at the `DISupprogam` flag `AllCallsDescribed`.
> >
> >
> > Something like D75175 <https://reviews.llvm.org/D75175> (and then rebase reland of the D73534 <https://reviews.llvm.org/D73534> on top of that).
>
>
> Does that patch make this one redundant, or can we land in the cases that this patch fixes with -O>0?


This patch fixes the problem and we need it for sure. But, the D75175 <https://reviews.llvm.org/D75175> is another idea (nothing to do with this patch) to avoid production of call site info in the case of non-debug builds, which makes sense to me, since it is being used only for Debug Entry Values feature, but we can move the discussion to that patch.



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:690
+  /// In addition to \p isCandidateForCallSiteEntry(), it checks for call
+  /// instructions inside bundles.
+  bool shouldUpdateCallSiteInfo() const;
----------------
vsk wrote:
> A better description might be: "Returns true if copying, moving, or erasing this instruction requires updating Call Site Info (see \ref copyCallSiteInfo, \ref moveCallSiteInfo, etc)."
+1
Thanks!


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:878
 
-  CallSiteInfoMap::iterator CSIt = getCallSiteInfo(Old);
-  if (CSIt == CallSitesInfo.end())
-    return;
+  auto BundleIt = MI->getIterator();
+  do {
----------------
dstenb wrote:
> Sorry, I missed this the first round, but perhaps it's worth making this a range-based loop?
> 
> ```
> for (auto &BMI : make_range(getBundleStart(MI->getIterator()),
>                             getBundleEnd(MI->getIterator())))
> ```
Sounds better.


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

https://reviews.llvm.org/D74904