[PATCH] D85012: [NOT FOR COMMIT(yet)] [deadargelim] Use entry values for unused args

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 6 18:01:18 PDT 2020


dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1843-1853
+    // We might already have generated the entry values within deadargelim,
+    // so we don't consider these as candidates for entry values anymore.
+    if (MI.isDebugValue() && !MI.isDebugEntryValue())
+      recordEntryValue(MI, DefinedRegs, OpenRanges, VarLocIDs);
   }
 
   // Initialize per-block structures and scan for fragment overlaps.
----------------
djtodoro wrote:
> dblaikie wrote:
> > djtodoro wrote:
> > > dblaikie wrote:
> > > > Hmm - how do we use entry values, if we don't know what parts of the parameter's range are valid? Does this only recover entry values for descriptions that are about to get clobbered here? Not for descriptions that became dead/were removed in previous optimization passes?
> > > > 
> > > > (yeah, guess that means we're missing a lot of entry value use, eg:
> > > > ```
> > > > void f1(int);
> > > > void f2(int i) {
> > > >   i = i + 5; // line 3
> > > >   f1(3);
> > > > }
> > > > ```
> > > > We get an entry value if line 3 is deleted, but otherwise we don't get one/only get a location that's valid up until the 'f1' call.
> > > > Similarly if line 3 was `int j = i + 5;` an entry_value location could be used to describe that but isn't. Oh, even if "int j = i;" is used, that doesn't get an entry value location.
> > > > 
> > > > I guess overall my point would be we should be creating entry_values a lot more in intermediate passes. 
> > > > 
> > > > Hmm, I wonder if we'd ever need to combine these strategies - but I guess not. If a location is already described with an entry value, no need to use another. Oh, if we do end up with the work to describe a variable's value with multiple IR Registers - then we might want this. ALso if we are able to use entry_value not at the top-level, then maybe this patch is incorrect? (maybe leave a FIXME about generalizing this for those cases? Could write IR test cases, well at least for the non-top-level: an entry_value + 5, for instance?kEven if no pass is creating such a thing right now, it's probably good/reasonable to support it at the IR level?)
> > > Actually, this piece of code here (on the line 1852) is unwanted (maybe), I need to test it.
> > > 
> > > In general, entry_value depends on call-site info (call_value attribute of corresponding call-site-param). We generate entry_values in the callee as a backup locations, and we don't know whether it will turn into real value. The more call-site-parameters the more of these will be real values.
> > > Currently, we  generate entry_values *only* for unmodified parameters. We can use entry_value op for modified params as well, if the modification could be expressed in terms of its entry value. There are TODOs within `isEntryValueCandidate()`. I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.
> > > Actually, this piece of code here (on the line 1852) is unwanted (maybe), I need to test it. 
> > 
> > OK - not sure I understand why it may or may not be wanted, though.
> > 
> > > In general, entry_value depends on call-site info (call_value attribute of corresponding call-site-param). We generate entry_values in the callee as a backup locations, and we don't know whether it will turn into real value. The more call-site-parameters the more of these will be real values.
> > 
> > Yep, I'm with you there.
> > 
> > > Currently, we generate entry_values *only* for unmodified parameters. We can use entry_value op for modified params as well, if the modification could be expressed in terms of its entry value.
> > 
> > We should be able to use them for non-parameters as well, eg:
> > ```
> > void f1(int i) {
> >   int j = i;
> >   i = 7;
> > }
> > ```
> > 
> > >  There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.
> > 
> > I'd have thought it'd be fairly generalized - usable at the IR level, but with the CodeGen level here used for cases where the code generation truncates a live range & we could fall back to an entry value. Though I suppose to do that we need to know that an entry value is usable for the given variable - which means tracking potentially two locations throughout the IR. ("here's the main location and here's an entry_value-based location you can use if you end up truncating or otherwise clobbering/shrinking the live range of the main location")
> > > Actually, this piece of code here (on the line 1852) is unwanted (maybe), I need to test it. 
> > 
> > OK - not sure I understand why it may or may not be wanted, though.
> > 
> We don't support fragmented entry values, so it is better to leave like this.
> 
> > > In general, entry_value depends on call-site info (call_value attribute of corresponding call-site-param). We generate entry_values in the callee as a backup locations, and we don't know whether it will turn into real value. The more call-site-parameters the more of these will be real values.
> > 
> > Yep, I'm with you there.
> > 
> > > Currently, we generate entry_values *only* for unmodified parameters. We can use entry_value op for modified params as well, if the modification could be expressed in terms of its entry value.
> > 
> > We should be able to use them for non-parameters as well, eg:
> > ```
> > void f1(int i) {
> >   int j = i;
> >   i = 7;
> > }
> > ```
> Defensively! Local variables may gain benefits as well.
> 
> > >  There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.
> > 
> > I'd have thought it'd be fairly generalized - usable at the IR level, but with the CodeGen level here used for cases where the code generation truncates a live range & we could fall back to an entry value. Though I suppose to do that we need to know that an entry value is usable for the given variable - which means tracking potentially two locations throughout the IR. ("here's the main location and here's an entry_value-based location you can use if you end up truncating or otherwise clobbering/shrinking the live range of the main location")
> Yes, something similar we did in the latest implementation within LiveDebugValues. Do you think it could be a separate IR pass (e.g. EntryValuePass) or (somehow) to extend variable's metadata and in every moment of the pipeline we pick the backup/entry_value when the primary location/value has been lost?
> 
> > > There are TODOs within isEntryValueCandidate(). I remember I tried to add support for that case here (D68973), but it was quick-made-patch, and I've never returned to it. My opinion is that such support would be easier for the implementation on the IR level, and we might want to implement it there.
> > 
> > I'd have thought it'd be fairly generalized - usable at the IR level, but with the CodeGen level here used for cases where the code generation truncates a live range & we could fall back to an entry value. Though I suppose to do that we need to know that an entry value is usable for the given variable - which means tracking potentially two locations throughout the IR. ("here's the main location and here's an entry_value-based location you can use if you end up truncating or otherwise clobbering/shrinking the live range of the main location")
> 
> Yes, something similar we did in the latest implementation within LiveDebugValues. Do you think it could be a separate IR pass (e.g. EntryValuePass) or (somehow) to extend variable's metadata and in every moment of the pipeline we pick the backup/entry_value when the primary location/value has been lost?

What I was picturing was extending the variable location metadata to describe two locations - eg: add an extra two-state enum parameter that could be used to encode entry_value-relative locations separate from/even overlapping with the non-conditional locations.

That way if a non-conditional location is cut short, we'd have the entry_value location to fallback on in some cases.

This is, of course, a very invasive change & not something to be done lightly (usual: if you're interested let's chat about this with the usual folks on llvm-dev, get some worked examples, etc)- but seems to me like what would be needed to get the most out of entry_values - maybe other folks have other ideas/directions, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85012



More information about the llvm-commits mailing list