Page MenuHomePhabricator

[NOT FOR COMMIT(yet)] [deadargelim] Use entry values for unused args
Needs ReviewPublic

Authored by djtodoro on Jul 31 2020, 2:41 AM.

Details

Summary

Currently we generate entry value only within LiveDebugValues, but it seems we can use it within DeadArgElim for unused arguments.

Addresses PR39716.

Diff Detail

Event Timeline

djtodoro created this revision.Jul 31 2020, 2:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2020, 2:41 AM
djtodoro requested review of this revision.Jul 31 2020, 2:41 AM
dblaikie added inline comments.Aug 3 2020, 1:17 PM
llvm/lib/CodeGen/LiveDebugValues.cpp
1843–1853

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?)

djtodoro added inline comments.Aug 3 2020, 11:48 PM
llvm/lib/CodeGen/LiveDebugValues.cpp
1843–1853

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.

dblaikie added inline comments.Aug 4 2020, 11:34 AM
llvm/lib/CodeGen/LiveDebugValues.cpp
1843–1853

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")

djtodoro added inline comments.Aug 5 2020, 12:21 AM
llvm/lib/CodeGen/LiveDebugValues.cpp
1843–1853

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?

dblaikie added inline comments.Aug 6 2020, 6:01 PM
llvm/lib/CodeGen/LiveDebugValues.cpp
1843–1853

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.

djtodoro added inline comments.Aug 7 2020, 1:52 AM
llvm/lib/CodeGen/LiveDebugValues.cpp
1843–1853

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.

1843–1853

Sure, we should be using entry_values more, even on IR in order to save variables locations/values. One possible solution may be extending DBG_VALUE to track the backup, so we can easily track modifications that could be expressed in terms of entry_values, but we'll see... I'll be away from keyboard for two weeks, but when I am back, I'll try out some possibilities and send/start the conversation on this topic on llvm-dev. Thanks!