Page MenuHomePhabricator

[FunctionAttrs] Annotate intrinsics with nosync
Needs ReviewPublic

Authored by sstefan1 on Jul 28 2019, 9:15 AM.

Details

Summary

With introduction of nosync function attribute, we should annotate intrinsics where nosync applies. Only few are annotated right now, any suggestins are welcome.

Event Timeline

sstefan1 created this revision.Jul 28 2019, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2019, 9:15 AM

I annotated some missing no-syncs when I realized you need to rebase to get the willreturn changes. Almost everything that is willreturn should be nosync too (except the mem* stuff).

llvm/include/llvm/IR/Intrinsics.td
323

no-sync

567

min/max no-sync

580

no-sync

585

no-sync

714

no-sync

720

no-sync

730

no-sync

1043

unrelated.

jdoerfert added inline comments.Jul 30 2019, 8:39 PM
llvm/test/Analysis/BasicAA/cs-cs.ll
368

Why is this change happening?

sstefan1 marked an inline comment as done.Jul 30 2019, 10:31 PM
sstefan1 added inline comments.
llvm/test/Analysis/BasicAA/cs-cs.ll
368

I am not really sure, that was the output and I haven't touched anything there.

jdoerfert added inline comments.Jul 30 2019, 11:05 PM
llvm/test/Analysis/BasicAA/cs-cs.ll
368

Rebase, make sure the src is clean, if it still is different, then there seems to be a problem somewhere (the function above doesn't feature intrinsics)

I almost think intrinsics should be assumed nosync by default. I'm not thrilled at the prospect of another attribute that nearly every intrinsic needs to be annotated with

I almost think intrinsics should be assumed nosync by default. I'm not thrilled at the prospect of another attribute that nearly every intrinsic needs to be annotated with

Mh, I'm not opposing that idea but also not convinced. Doing it for all makes it easier to introduce errors I guess. Do we have precedence?

If we want to do this, maybe we should make a list of attributes that need to be explicitly removed for intrinsics.
I expect not only nosync to be on it but also others like willreturn, nofree, nounwind, etc.

arsenm added a comment.Aug 2 2019, 1:33 PM

I almost think intrinsics should be assumed nosync by default. I'm not thrilled at the prospect of another attribute that nearly every intrinsic needs to be annotated with

Mh, I'm not opposing that idea but also not convinced. Doing it for all makes it easier to introduce errors I guess. Do we have precedence?

If we want to do this, maybe we should make a list of attributes that need to be explicitly removed for intrinsics.
I expect not only nosync to be on it but also others like willreturn, nofree, nounwind, etc.

It probably should be explicit, but I feel like I'm in a perpetual state of adding attributes to intrinsic definitions and it's never complete

I almost think intrinsics should be assumed nosync by default. I'm not thrilled at the prospect of another attribute that nearly every intrinsic needs to be annotated with

Mh, I'm not opposing that idea but also not convinced. Doing it for all makes it easier to introduce errors I guess. Do we have precedence?

If we want to do this, maybe we should make a list of attributes that need to be explicitly removed for intrinsics.
I expect not only nosync to be on it but also others like willreturn, nofree, nounwind, etc.

It probably should be explicit, but I feel like I'm in a perpetual state of adding attributes to intrinsic definitions and it's never complete

I would propose the following:

  1. Let's go ahead with this
  2. We ask on the list how people feel about "opt-out" attributes for intrinsics and go from there

I almost think intrinsics should be assumed nosync by default. I'm not thrilled at the prospect of another attribute that nearly every intrinsic needs to be annotated with

Mh, I'm not opposing that idea but also not convinced. Doing it for all makes it easier to introduce errors I guess. Do we have precedence?

If we want to do this, maybe we should make a list of attributes that need to be explicitly removed for intrinsics.
I expect not only nosync to be on it but also others like willreturn, nofree, nounwind, etc.

It probably should be explicit, but I feel like I'm in a perpetual state of adding attributes to intrinsic definitions and it's never complete

I would propose the following:

  1. Let's go ahead with this
  2. We ask on the list how people feel about "opt-out" attributes for intrinsics and go from there

I agree. FWIW, I'd be more comfortable with some TableGen way of defining common groups of properties, giving these groups a single name, and then use that property-group name on most of the intrinscs.

arsenm added a comment.Aug 2 2019, 9:18 PM

I think nounwind is the one precedent case. There’s no explicit annotation for it, but it’s assumed for all but one intrinsic

I think nounwind is the one precedent case. There’s no explicit annotation for it, but it’s assumed for all but one intrinsic

So, the Attributor does not use any special handling for intrinsics when it comes to nounwind.
I don't think we should. Actually, the more I think out it, I believe an "opt-out" list if attributes would be the right thing.
We just need some consensus on that.

@sstefan1 Would you like to write an email to the list asking what people think about this idea? Feel free to email me and @arsenm a draft of the email if you want feedback.