Page MenuHomePhabricator

[Attributor] Introduce Attribute seed allow list.
ClosedPublic

Authored by kuter on Jul 5 2020, 5:21 PM.

Details

Summary

This patch introduces a command line option for allowing only selected attributes to be seeded.

Diff Detail

Event Timeline

kuter created this revision.Jul 5 2020, 5:21 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kuter updated this revision to Diff 275585.Jul 5 2020, 5:30 PM

Replace ternary expression with early return

Can you add a test using this option?

llvm/lib/Transforms/IPO/Attributor.cpp
1465

would it make sense to make this always check lower case names, to avoid mistakes?

kuter updated this revision to Diff 275688.Jul 6 2020, 5:57 AM

Introduce test for the command line option

kuter marked an inline comment as done.Jul 6 2020, 5:59 AM
kuter added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
1465

Since the name is usually the name of the class that defines the attribute,
I think it makes for sense the use the name as is.

uenoku added a comment.Jul 6 2020, 2:28 PM

Either is fine but I think it is more natural to forbid an empty list.

kuter added a comment.Jul 6 2020, 3:02 PM

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

uenoku added a comment.EditedJul 6 2020, 3:23 PM

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Yes, I think replacing ZeroOrMore with OneOrMore is enoguh

kuter added a comment.Jul 6 2020, 3:27 PM

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Yes, I think replacing ZeroOrMore with OneOrMore is enoguh

I think cl::OneOrMore causes a error to be generated if the option is not specified.
That would make it mandatory to have a atleast one --attributor-seed-allow-list parameter for every invocation of
opt

https://llvm.org/docs/CommandLine.html

The cl::OneOrMore modifier indicates that the option must be specified at least one time.

uenoku added a comment.Jul 7 2020, 5:56 AM

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Yes, I think replacing ZeroOrMore with OneOrMore is enoguh

I think cl::OneOrMore causes a error to be generated if the option is not specified.
That would make it mandatory to have a atleast one --attributor-seed-allow-list parameter for every invocation of
opt

https://llvm.org/docs/CommandLine.html

The cl::OneOrMore modifier indicates that the option must be specified at least one time.

Yeah, I might be wrong. Sorry for confusion.

As discussed, we can check for the validity of the list in the constructor. Two more comments below.

llvm/include/llvm/Transforms/IPO/Attributor.h
1432

Use more words to describe this please.

Nit: Maybe initialize it directly here.

llvm/test/Transforms/Attributor/allow_list.ll
38

Get rid of all lines we don't need, e.g. the last 5.

kuter updated this revision to Diff 277247.Jul 11 2020, 9:54 AM
  1. Better comments.
  2. Remove unneeded lines from the test.

I investigated the situation with the argument option that this patch introduces.
LLVM CommandLine library already does not allow a empty list but what happens is when you
pass a empty list for a execution like this opt --attribute-seed-allow-list
CommandLine automatically displays an error and terminates execution.
But when there are other arguments after the option the then that option becomes a member of the list.

It would have been great if we had a way of telling whether a name is valid or not.
Perhaps we could have a list of valid attribute names.

jdoerfert accepted this revision.Jul 11 2020, 10:36 AM

It would have been great if we had a way of telling whether a name is valid or not. Perhaps we could have a list of valid attribute names.

Let's not worry about that too much now. It seems an empty list doesn't make sense and when people pass garbage they'll get something.

One nit below and one idea but otherwise LGTM.

We could set the seeding to false if the list is empty to avoid looking up if it is empty all the time. Feel free to ignore.

llvm/include/llvm/Transforms/IPO/Attributor.h
1432

Nit: called. \see ...

This revision is now accepted and ready to land.Jul 11 2020, 10:36 AM
kuter closed this revision.Jul 12 2020, 4:32 AM