This is an archive of the discontinued LLVM Phabricator instance.

Reword lifetime description to avoid contradicting long term implementation
AbandonedPublic

Authored by reames on Mar 24 2021, 2:51 PM.

Details

Summary

This is effectively a rework of c821ef451. That change had a major problem in that it contradicted our long standing dereferenceability implementation and turned LICM into an unsound transform. On the other hand, some of the wording clarifications were helpful. So, instead of a whole sale revert, I'm posting a potential alternative.

The core notions here are: 1) separate dereferenceability and object lifetime (in the marker intrinsic sense), and 2) unify handling for all memory object types.

If you read the existing StackColoring.cpp implementation, the degenerate slot section is describing something close to the proposed split here.

I added parties I know to be interested in this area as reviewers. I would appreciate if one of you would pick up this patch and drive it. I don't actually care about the lifetime markers at all. I'm simply posting this to try and drive the discussion into a productive direction.

Diff Detail

Event Timeline

reames created this revision.Mar 24 2021, 2:51 PM
reames requested review of this revision.Mar 24 2021, 2:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 2:51 PM
aqjune added a comment.EditedMar 24 2021, 9:13 PM

Hi,

The core notions here are: 1) separate dereferenceability and object lifetime (in the marker intrinsic sense), and 2) unify handling for all memory object types.

I think it is good to separate the two concerns: the second issue already led a long discussion in llvm-dev (this Jan), and it didn't converge as well.
The first issue seems important ATM, so let's make this patch concentrate on that. This will help converge discussion shortly.
It seems enough to update "Object Lifetime" section to address the dereferenceability issue, what do you think?

D99135 and the llvm-commit mails have many valuable issues recorded about dereferenceability of a dead stack-allocated object, so I'd prefer shortly mentioning the issues here to not lose it.
Especially, store to an stack-allocated object that is dead may overwrite another stack object's value <- this seems pretty important to me.

llvm/docs/LangRef.rst
2591

I think this dereferenceability issue is more related to stack-allocated objects than heap/global objects. I couldn't write a program/find an existing optimization that has the dereferenceability issue with heap/global objects. Do you have any?

If it is, what do you think about changing this paragraph instead of the above paragraph to say everything that should be addressed? This paragraph is a good place to talk about stack thingy.

Macro point: please consider everything in this response as advisory, and non-blocking. As mentioned in the related discussion in the commit thread for the original patch, I am planning on removing myself from this discussion going forward, and nothing said here should be considered blocking. If it's useful, great. If not, feel free to ignore.

Hi,

The core notions here are: 1) separate dereferenceability and object lifetime (in the marker intrinsic sense), and 2) unify handling for all memory object types.

I think it is good to separate the two concerns: the second issue already led a long discussion in llvm-dev (this Jan), and it didn't converge as well.

I'm not sure this is really possible. The example which concerns me is what if we have a lifetime end buried in a callee? As the current semantics are written, the behavior of such a call depends on the underlying allocation type. This makes it really hard to either a) reason about the semantics of the function in isolation, or b) perform optimization if we're forced to assume the conservative union of the handling.

The first issue seems important ATM, so let's make this patch concentrate on that. This will help converge discussion shortly.
It seems enough to update "Object Lifetime" section to address the dereferenceability issue, what do you think?

I am in general very hesitant about this.

My primary concern is not that the semantics are self consistent or not. My concern is that the semantics seem not to match the existing implementation in the optimizer. (Specifically, see the hoisting example I gave elsewhere.)

I can see three possible paths to resolution:

  1. change langref in manner to match implementation
  2. change implementation to match new langref
  3. make an argument that case described is impossible to construct (and thus implementation does actually match new langref wording)

I don't have any preference as to which path is taken. I'd tend to default to option 1, but that's just what I'd do, not the only reasonable path forward.

D99135 and the llvm-commit mails have many valuable issues recorded about dereferenceability of a dead stack-allocated object, so I'd prefer shortly mentioning the issues here to not lose it.
Especially, store to an stack-allocated object that is dead may overwrite another stack object's value <- this seems pretty important to me.

Fair, I don't really have a good suggestion as to how to resolve this.

reames abandoned this revision.Mar 25 2021, 12:08 PM

Abandoning change. As noted in the original commit thread, I'm removing myself from further discussion on this topic. Feel free to use anything in this change set which is useful.