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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/utils/TableGen/CodeGenTarget.cpp | ||
---|---|---|
770–771 | I think you could avoid touching tablegen by implementing a filter in the tablegen source |
llvm/utils/TableGen/CodeGenTarget.cpp | ||
---|---|---|
770–771 | 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? |
llvm/utils/TableGen/CodeGenTarget.cpp | ||
---|---|---|
770–771 | 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. |
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.
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 |
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
354 | I'm little bit confused. Are you suggesting going back to the original approach? |
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 |
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
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.
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.
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?
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?
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
344 | Should add a comment documenting this |
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.
This should be spilt to a separate change