Missing LangRef documentation for nocallback

[nhaehnle]

As the title says: there's a nocallback attribute but zero mentions of it in LangRef or anywhere else in the llvm/docs/ folder.

Digging in the history a bit, it seems like it's related to __attribute__((leaf)). The lack of LangRef documentation was noted in the Phabricator review in 2020 and again in 2021 and 2022 but has apparently still not been addressed.

cc @gulfemsavrun @jdoerfert @nunoplopes

[nikic]

There is a LangRef patch in https://reviews.llvm.org/D131628, but there are disagreements about the semantics.

[nunoplopes]

The main problem is that since the clang leaf attribute is mapped one-to-one to the LLVM attribute, we have to follow gcc's semantics.
But the gcc folks seem to have ignored our bug report. So, we don't know what's the semantics of gcc's attribute.

So, either we implement a safe version of the semantics, or we go with the less safe semantics w.r.t. to IPO and document that in clang's documentation.
But I feel that work should be done by the authors of the LLVM patches. They should fix this mess ASAP. We can't have undocumented stuff. That's a good recipe for disaster.

[davidbolvansky]

This is not how things should work, so https://reviews.llvm.org/D90275 should be reverted.

[davidbolvansky]

I'll revert this patch on Friday if no one chimes in. Thank you!

Please go ahead @nunoplopes and remove this undocumented attribute.

[gulfemsavrun]

The main problem is that since the clang leaf attribute is mapped one-to-one to the LLVM attribute, we have to follow gcc's semantics. But the gcc folks seem to have ignored our bug report. So, we don't know what's the semantics of gcc's attribute.

So, either we implement a safe version of the semantics, or we go with the less safe semantics w.r.t. to IPO and document that in clang's documentation. But I feel that work should be done by the authors of the LLVM patches. They should fix this mess ASAP. We can't have undocumented stuff. That's a good recipe for disaster.

Just to clarify: I implemented leaf attribute front-end support as a starter task to get more familiar with Clang/LLVM. When I implemented it, there was not a concern about the semantics of that attribute. Adding it into the LangRef Manual is overlooked. So, it was not meant to create a mass in the first place. But, a discussion has recently started about the semantics of the attribute in https://reviews.llvm.org/D131628. We can revert https://reviews.llvm.org/D90275 if you folks think that this is a better approach to proceed.

[nunoplopes]

I'm sorry you had such a first experience. It wasn't supposed to be like that.
The patch shouldn't have been approved in the first place without LangRef documentation at least.
Sometimes the things that seem trivial are not.. And semantics of LLVM IR is certainly not an easy place to start.

[mysterymath]

Please note that it's no longer going to be very straightforward to revert https://reviews.llvm.org/D90275: the nocallback attribute has worked its way into a fair number of LLVM passes since 2020. It seems like it would be better to continue the semantics discussion, but document LLVM's current behavior in the mean time.

[jdoerfert]

I'll revert this patch on Friday if no one chimes in. Thank you!

Please go ahead @nunoplopes and remove this undocumented attribute.

I doubt it is as easy as you make this sound (as @mysterymath noted).
It's not like you can remove https://reviews.llvm.org/D90275 and call it a day,
There are tests all over the place, intrinsics, optimizations, etc. depending on this.

That said, we also have a community agreement against arbitrary deadlines.
I mean, we have been over this multiple times by now. Setting yet another
one week deadline is not helpful at all, especially if the discussion on it
is in a bug report only. Please follow courtesy procedures which include enough
time >= 2 weeks, a proper announcement, and addressing existing objections.
Since I have arguably objected enough in the earlier discussion
(https://reviews.llvm.org/D131628), one can assume I will continue to do so and
should not simply ignore that.

Now to the problem at hand:
We need lang ref documentation, I totally agree with this (see https://reviews.llvm.org/D90275#2454912).
Earlier, we proposed two constructive ways out of this (https://reviews.llvm.org/D131628#3732352).
The first one now has a patch (https://reviews.llvm.org/D137360).
With the addition of an escape hatch to said patch, I think we have satisfied all concerns
and can land D137360 as well as D131628 any time now. Agreed?

[gulfemsavrun]

I have landed https://reviews.llvm.org/D131628 and https://reviews.llvm.org/D137360 to fix this issue.