Page MenuHomePhabricator

Add Query API for llvm.assume holding attributes
ClosedPublic

Authored by Tyker on Jan 16 2020, 3:10 PM.

Diff Detail

Event Timeline

Tyker created this revision.Jan 16 2020, 3:10 PM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert added inline comments.Jan 16 2020, 7:06 PM
llvm/include/llvm/Transforms/Utils/KnowledgeRetention.h
75

Documentation missing.

llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
191

Commonly we have messages in an assert, also below.

216

I mentioned in D72475 now that the name should not be a part of the key as it can change.

243

The above seems complicated. Can you use helpers or comments to make it easier to read?

270

Where is this used?

Tyker updated this revision to Diff 238910.Jan 17 2020, 3:31 PM
Tyker marked 4 inline comments as done.

addressed comments.

llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
243

i added and used a helper to simplify this and other pieces of code is it enough ?

270

this is used for testing ShouldPreserveAllAttributes option from unittests.

I won't review this properly till next week. Generally looks good so you can work on the next patch if you want to.

llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
255

What is the use case for IsOn = nullptr? If there is none, we should make it a reference so it's obvious.

270

If it's only used in the unit test can we move it there?

Tyker marked 2 inline comments as done.Jan 21 2020, 3:40 PM

I won't review this properly till next week. Generally looks good so you can work on the next patch if you want to.

i started working on ignoring or dropping uses in llvm.assume when a transformation is use sensitive.

Also because of that, and we do not have settled on this scheme in the RFC thread.

i would like to have confirmation that the operand bundles representation is what we are going for ?
i don't recall it being a point of contention but it wasn't explicitly approved. and you mentioned in that the issue as not settled.

llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
255

currently IsOn = nullptr is for attributes that where on the call of a function or the declaration of a function.

this case only occurs when ShouldPreserveAllAttributes is set.

270

it needs to have access to the ShouldPreserveAllAttributes flag. which we probably shouldn't make global.

I won't review this properly till next week. Generally looks good so you can work on the next patch if you want to.

i started working on ignoring or dropping uses in llvm.assume when a transformation is use sensitive.

Great! Can you share it on phab so I can reference it in my email (see below).

Also because of that, and we do not have settled on this scheme in the RFC thread.

i would like to have confirmation that the operand bundles representation is what we are going for ?
i don't recall it being a point of contention but it wasn't explicitly approved. and you mentioned in that the issue as not settled.

Correct. I'll send another email today, describing our progress and hoping ppl agree this is a good solution :)

llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
255

I would have expected the IsOn to be the called value or respectively function then. Is there a reason for one or the other?

270

Now I understand but I still think this is problematic.

What if we add a second call into KnowledgeRetention.h where the value of ShouldPreserveAllAttributes is passed in directly. The existing interface will call that internally and use the command line flag value. Instead of the command line flag value, existing uses will then use the new argument. That way we can determine the value from the call site or via the command line.

What do you think?

I won't review this properly till next week. Generally looks good so you can work on the next patch if you want to.

i started working on ignoring or dropping uses in llvm.assume when a transformation is use sensitive.

Great! Can you share it on phab so I can reference it in my email (see below).

i added it to phab (D73404) after cleaning it up. sorry i toke soo long i haven't had the time i wanted to work on this.

Also because of that, and we do not have settled on this scheme in the RFC thread.

i would like to have confirmation that the operand bundles representation is what we are going for ?
i don't recall it being a point of contention but it wasn't explicitly approved. and you mentioned in that the issue as not settled.

Correct. I'll send another email today, describing our progress and hoping ppl agree this is a good solution :)

thanks for the mail.

Tyker marked an inline comment as done.Jan 24 2020, 11:59 PM
Tyker added inline comments.
llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
255

i am staring to think the attribute on call and function is perhaps too small of a use case to be worth supporting. i haven't yet found any other use case than the cold attribute that said i haven't be looking for it.

but the idea was that Users of that API wouldn't need to care or know what function was called here. but just that this function has X attribute. i think it loose its last bit of usefulness if users need to know ahead of time that there was a calls to function F before querying it.

jdoerfert added inline comments.Jan 25 2020, 12:43 PM
llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
255

I can see your argument. Maybe the "map API" will allow your use case even if we retain the functions that carried the attributes.
I am hesitant to throw away information here if there is another way. When we query *all* information the assume stores in a map the keys could either be attributes or values (probably both make sense). In the keys = attributes version the user can look for "cold" and act on it. Arguably, we don't even need the map API for this. We can have a define that if IsOn is null any occurrence of the attribute is returned even though we store the original "IsOn" value internally.

Tyker retitled this revision from [WIP] Add Query API for llvm.assume holding attributes to Add Query API for llvm.assume holding attributes.Feb 3 2020, 1:44 PM
jdoerfert added inline comments.Feb 12 2020, 8:03 AM
llvm/include/llvm/Transforms/Utils/KnowledgeRetention.h
81

This is not needed anymore, or is it? (I hope we can replace it somehow if it is.)

llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
69

Typo.

Tyker updated this revision to Diff 244707.Feb 14 2020, 10:46 AM

addressed comments.

This looks for with minor comments and one remaining question: Do we use the value name in the sorting? If so, are we sure arbitrary renaming of values will not be a problem? If so, does it help to use the value name in the key?

llvm/include/llvm/Transforms/Utils/KnowledgeRetention.h
60

Nit: Can you move the enum class (with a short doc) above the documentation of the function. It is a bit confusing this way and I'm not sure Doxygen will pick up on it they way we want it to.

llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
88

This still uses the value name in the key, right? Does this work if the values are renamed arbitrarily?

194

Nit: Why do we allow None here?

205

Nit: Move this down closer to the use sites. Maybe also replace Elem with Value to make it clear it returns an llvm value.

Tyker updated this revision to Diff 245016.EditedFeb 17 2020, 11:21 AM
Tyker marked 4 inline comments as done.

addressed comments

the Name is used in the key to prevent non-deterministic order of properties in llvm.assume.

the DenseSet used for uniquing, to prevent reliance on the actual hashing implementation is randomized on each run of the program. so if we don't fully order element as we do BuildAssumeFromInst can build llvm.assume where the order of constaint in the bundle may change from run to run. this is not usually visible. but this is going to be an issue for the tests as they are usually simple string matches on the output.

Tyker added inline comments.Feb 17 2020, 11:21 AM
llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
88

The Name is still used in the key. but this is only to get a full ordering without depending on pointer values or other sources of non-determinism. the lookup algorithm doesn't depend on Names.

if there is an other deterministic source that can be used for ordering that you would feel more comfortable with, it can be used instead.

for example the current unit test passes when we use the reverse of the name. also unit test have a test with replaceAllUsesWith.

Tyker updated this revision to Diff 245019.Feb 17 2020, 11:38 AM
jdoerfert accepted this revision.Feb 17 2020, 5:11 PM

addressed comments

the Name is used in the key to prevent non-deterministic order of properties in llvm.assume.

the DenseSet used for uniquing, to prevent reliance on the actual hashing implementation is randomized on each run of the program. so if we don't fully order element as we do BuildAssumeFromInst can build llvm.assume where the order of constaint in the bundle may change from run to run. this is not usually visible. but this is going to be an issue for the tests as they are usually simple string matches on the output.

[Apologies for the wall of text, I wrote down my thoughts as I think it might help (in the future).]

At first this made sense to me and put my worries to rest. I agree that if we only use the names to make it deterministic but not rely on them in any other way we should be OK. That said, it is not deterministic in the absence of names, correct? I mean, if we remove the names of *all* variables, every getName() invocation will return the empty string. While determinism is a describable property for testing, it is crucial for compilation in general. However, I think the compilation part is not a problem right now as we can only query the extreme values (largest/lowest) for a pair attribute + value. In which order all the operand bundles that store a value for such a pair are internally doesn't matter, the largest/lowest is always the same. Now for testing I am also not too worried. We can test with the CHECK-DAG feature if we have ambiguous cases or force values to have names to make it deterministic.

Long story short, LGTM.

This revision is now accepted and ready to land.Feb 17 2020, 5:11 PM

Now we need to query the operand bundle information somewhere and actually emit it ;)

This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Feb 21 2020, 9:08 AM
nikic added inline comments.
llvm/lib/Transforms/Utils/KnowledgeRetention.cpp
232

This causes a warning:

warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

Can you please check whether this condition can be simply dropped, or some other change is needed?