Page MenuHomePhabricator

intrinsics attribute opt-out list proposal.
ClosedPublic

Authored by sstefan1 on Nov 17 2019, 2:40 PM.

Details

Summary

This patch introduces an opt-out attribute list for intrinsics, discussed in D65377.
This should make it easier to annotate intrinsics with attributes that apply to most of them.

Diff Detail

Event Timeline

sstefan1 created this revision.Nov 17 2019, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2019, 2:40 PM
arsenm added inline comments.Nov 17 2019, 9:08 PM
llvm/utils/TableGen/CodeGenTarget.cpp
778–779

I think you could avoid touching tablegen by implementing a filter in the tablegen source

sstefan1 marked an inline comment as done.Nov 18 2019, 11:51 AM
sstefan1 added inline comments.
llvm/utils/TableGen/CodeGenTarget.cpp
778–779

I'm not really sure how to do that in tablegen source. Not really used beyond adding properties to intrinsics.

Is there a reason for avoiding current approach?

arsenm added inline comments.Nov 18 2019, 8:37 PM
llvm/utils/TableGen/CodeGenTarget.cpp
778–779

You could do something like:

class Intrinsic {
  IntrProperties
  OutOutIntrProperties
   IntrPropertiesImpl = !foldl(something overly complicated to filter out OutIntrProperties)

}

I'm pretty confident we have all the necessary mechanics in tablegen to implement this, but it would be ugly. We probably don't have a nice way to test if an element is in a list, but it's possible.

I think just generally avoiding hardcoded things in tablegen is a good idea.

sstefan1 updated this revision to Diff 272815.Jun 23 2020, 1:47 PM
  • revisit this patch with different implementation

Sorry this took so long.

I've implemented some sort of filter in tablegen source.

I've modified llvm.memset as an example how the opt-out list would look like. If this approach seems reasonable, I'll look what other tests have changed and need to be updated.

arsenm added inline comments.Jun 24 2020, 8:00 AM
llvm/include/llvm/IR/Intrinsics.td
122

This should be spilt to a separate change

354

I think this strategy doesn't actually work, given the prevalence of the error prone global let blocks. I've fixed many bugs from let Soemthing in :{} sections overwriting the inner members. I think the only way short of redesigning the language to have some kind of additive let to add to lists, I think the safest way to do this is to have tablegen handle the opt-out list

sstefan1 marked an inline comment as done.Jun 24 2020, 8:31 AM
sstefan1 added inline comments.
llvm/include/llvm/IR/Intrinsics.td
354

I'm little bit confused. Are you suggesting going back to the original approach?

arsenm added inline comments.Jun 24 2020, 8:44 AM
llvm/include/llvm/IR/Intrinsics.td
354

Yes, but I can't really decide which is better. Overriding class fields is generally a problem in tablegen though

sstefan1 updated this revision to Diff 274109.Jun 29 2020, 7:09 AM

Introduction of IsDefault field in IntrinsicProperty as suggested by @nhaehnle.

For now I've only set IntrNoSync to be default and changed memset to opt-out of IntrNoSync just as an example. Maybe the part where attributes are actually made default should be done in another patch?

Is there anyone else that should be added as a reviewer?

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

When we settled on a scheme we need to send another email telling people we'll activate it for NoSycn "soon" and they should check their target intrinsics appropriately. For the non-target ones, opt-out of NoSync for the following intrinsics (I mention part of their name), roughly in order: gc (garbage collection), objc (because I don't know any better), read/write register, instprof, _eh_ (because I don't know any better), again _gc_, _coro_ (maybe), deoptimize, _clear_cache (as it probably defeats the purpose), hwasan, xray, *atomic

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

Agreed, that could be another way to go about this. By now, I'm not really sure what is the best out of all the options. What do others think?

When we settled on a scheme we need to send another email telling people we'll activate it for NoSycn "soon" and they should check their target intrinsics appropriately.

Yes, that's why I suggested leaving that for another patch.

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

Probably depends on what the initial list looks like. I guess something like "nosync, norecurse, nounwind, willreturn" is reasonable for most intrinsics. Given that list, probably an intrinsic that wants to opt-out of one of those will want to opt-out of all of them.

On a related note, we might want to consider doing something for pointer/memory handling as well, since that's currently pretty awkward. Most pointer arguments should be nocapture/nofree, but we don't really mark that consistently.

On a related note, we might want to consider doing something for pointer/memory handling as well, since that's currently pretty awkward. Most pointer arguments should be nocapture/nofree, but we don't really mark that consistently.

I guess something similar can be done with pointer arguments as well. I can take a look at that once we settle on the approach here.

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

Probably depends on what the initial list looks like. I guess something like "nosync, norecurse, nounwind, willreturn" is reasonable for most intrinsics. Given that list, probably an intrinsic that wants to opt-out of one of those will want to opt-out of all of them.

That was what I would expect too. The above list might be extended with nofree but conceptually we might want to start with a single boolean to opt-out of everything. As we move on, we might go for a list or "categories", let's see.

@sstefan1 could u update the patch? We then send another email out with the list of 5 attributes we will make opt-out for intrinsics. A week or so later, assuming no one objected too much, we'll land it. In the meantime we need to update the target independent ones to opt-out appropriately. I guess my nosync list will catch almost all of them already but we'll have to give it another go as the patch is updated. WDYT?

sstefan1 updated this revision to Diff 275189.Jul 2 2020, 11:58 AM

DisableDefaultAttributes flag instead of opt-out list

@sstefan1 could u update the patch? We then send another email out with the list of 5 attributes we will make opt-out for intrinsics. A week or so later, assuming no one objected too much, we'll land it. In the meantime we need to update the target independent ones to opt-out appropriately. I guess my nosync list will catch almost all of them already but we'll have to give it another go as the patch is updated. WDYT?

I was under the impression that the attributes will be made default in another patch. Right now we don't opt out of the default attributes, but none are set to be default yet.
If it is better to do this in this patch, no problem. I can send an email, on Monday maybe?

sstefan1 updated this revision to Diff 275197.Jul 2 2020, 12:33 PM

small fix

looks good, we should get the follow up ready that enables 1-5 default attributes

Ping.
Can we move forward with this approach?

arsenm added inline comments.Aug 17 2020, 9:47 AM
llvm/include/llvm/IR/Intrinsics.td
344

Should add a comment documenting this

sstefan1 updated this revision to Diff 286085.Aug 17 2020, 10:46 AM

add comment for DisableDefaultAttributes flag.

sstefan1 updated this revision to Diff 286095.Aug 17 2020, 11:21 AM

update affected tests.

sstefan1 edited the summary of this revision. (Show Details)Aug 17 2020, 11:24 AM
arsenm accepted this revision.Aug 18 2020, 11:26 AM
This revision is now accepted and ready to land.Aug 18 2020, 11:26 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added a subscriber: mehdi_amini.EditedAug 19 2020, 10:08 PM

After this patch, llvm-tblgen in debug mode is 10x slower:
Before:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m1.238s
user	0m0.970s
sys	0m0.274s

After:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m11.671s
user	0m11.607s
sys	0m0.067s
jdoerfert reopened this revision.Aug 19 2020, 10:29 PM

After this patch, llvm-tblgen in debug mode is 10x slower:

Let's fix this properly. Reverted in 2f38c755ba46d1357d79c922966d964694eb854e

This revision is now accepted and ready to land.Aug 19 2020, 10:29 PM
jdoerfert requested changes to this revision.Aug 19 2020, 10:29 PM
This revision now requires changes to proceed.Aug 19 2020, 10:29 PM
sstefan1 updated this revision to Diff 286729.Aug 20 2020, 12:56 AM

fix slowdown issue

After this patch, llvm-tblgen in debug mode is 10x slower:
Before:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m1.238s
user	0m0.970s
sys	0m0.274s

After:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m11.671s
user	0m11.607s
sys	0m0.067s

Thanks for reporting this.
Numbers are similar in the release mode as well. The problem was that we go through all IntrProperties for every Intrinsic in search for ones that are default. However that can be done only once.

With reverted patch time in release was 9.43s for me, now it is 0.16s.

I'd need another approval from @arsenm or @nhaehnle.

Ping.

Does the fix for the performance issue seem ok?

This revision is now accepted and ready to land.Aug 25 2020, 10:50 AM
This revision was landed with ongoing or failed builds.Aug 26 2020, 2:41 AM
This revision was automatically updated to reflect the committed changes.