Page MenuHomePhabricator

intrinsics attribute opt-out list proposal.
Needs ReviewPublic

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.
Right now I added NoFree and NoSync. If this looks ok, I'll also replace "Throws" IntrinsicProperty with IntrNoUnwind
and make it default.

Note: This is just an attempt, I also intend to send a RFC on the list.

Diff Detail

Unit TestsFailed

TimeTest
30 mswindows > LLVM.TableGen::intrin-side-effects.td
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-tblgen.exe -gen-intrinsic-impl -I C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen/../../include C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrin-side-effects.td | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrin-side-effects.td
20 mswindows > LLVM.TableGen::intrinsic-long-name.td
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-tblgen.exe -gen-intrinsic-enums C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-long-name.td | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-long-name.td
30 mswindows > LLVM.TableGen::intrinsic-pointer-to-any.td
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-tblgen.exe -gen-intrinsic-impl C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-pointer-to-any.td | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-pointer-to-any.td
40 mswindows > LLVM.TableGen::intrinsic-struct.td
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-tblgen.exe -gen-intrinsic-enums C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-struct.td | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-struct.td
40 mswindows > LLVM.TableGen::intrinsic-varargs.td
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-tblgen.exe -gen-intrinsic-impl -I C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen/../../include C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-varargs.td | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\TableGen\intrinsic-varargs.td
View Full Test Results (6 Failed)

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
758–759

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
758–759

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
758–759

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.Tue, Jun 23, 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.Wed, Jun 24, 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.Wed, Jun 24, 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.Wed, Jun 24, 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.Mon, Jun 29, 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.Thu, Jul 2, 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.Thu, Jul 2, 12:33 PM

small fix

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