Page MenuHomePhabricator

[GlobalOpt/GlobalStatus] Handle GlobalVariables passed as function call operands with access attributes
Needs ReviewPublic

Authored by ddcc on Mar 26 2020, 4:30 PM.

Details

Summary

Model the inaccessiblememonly, readnone, readonly, and writeonly attributes on function calls and parameters for GlobalOpt and GlobalStatus.

Diff Detail

Unit TestsFailed

TimeTest
20 msMLIR.Dialect/Affine::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/Dialect/Affine/loop-permute.mlir -test-loop-permutation="permutation-map=1,2,0" | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/Dialect/Affine/loop-permute.mlir --check-prefix=CHECK-120

Event Timeline

ddcc created this revision.Mar 26 2020, 4:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 4:30 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl resigned from this revision.Mar 26 2020, 6:51 PM

I don't really know enough about this to give a meaningful review.

llvm/lib/Transforms/IPO/GlobalOpt.cpp
1837

Loads.push_back({CB, U->getType()->getPointerElementType()});

ddcc updated this revision to Diff 253034.Mar 26 2020, 8:12 PM

Remove explicit std::make_pair

jdoerfert requested changes to this revision.Mar 26 2020, 10:26 PM

Thanks for working on this, I'm really hoping we start to use attributes more aggressively. I inlined comments.

llvm/lib/Transforms/IPO/GlobalOpt.cpp
1828

This assert would need a message but it has to go completely. Having non-arg-operand uses is totally fine. That said, we should have a test case. One example where this will soon happen in practise is llvm.assume. Create a test with something like:
call void @llvm.assume(i1 true) ["align"(@Global, i64 128)]
If the use is not an arg operand you have to bail. Except if the User isDroppable().

1880

This doesn't work for calls unfortunately. We don't have an attribute that says it is accessed and how much of it is (yet!). My suggestion is a TODO and the following handling for now:
Write-only uses in calls are ignored, as read-none uses are. Read-only uses are matched with the type of the global value, thus we pretend we might read the entire thing.

llvm/lib/Transforms/Utils/GlobalStatus.cpp
205

This handles operand bundle uses wrong. We need to bail for them (I guess), except if the user isDroppable (maybe).


If the argument is not marked nocapture you have to assume everything could happen as you cannot track uses anymore.

This revision now requires changes to proceed.Mar 26 2020, 10:26 PM
ddcc updated this revision to Diff 253202.Mar 27 2020, 1:00 PM

Revise based on feedback

ddcc added a comment.Mar 27 2020, 1:05 PM

Sure, I'm working on a instrumentation pass which is inserting calls that inhibit optimization, so I'm trying to work around the issue using function attributes, and need to look into memory to register promotion next.

I've made the changes, but I'm a little confused why read-only and write-only calls need to be handled differently. Why can't I assume that in the worst-case, the entire type is accessed? Also, what are the semantics of readonly and writeonly with respect to pointer casts? Isn't it valid behavior in C to cast any pointer type to void * as long as it is casted back to the original type before being accessed, so wouldn't this affect both loads and stores (that the actual load/store size could be larger than the argument type size)?

ddcc updated this revision to Diff 253209.Mar 27 2020, 1:35 PM

Fix test failures

Sure, I'm working on a instrumentation pass which is inserting calls that inhibit optimization, so I'm trying to work around the issue using function attributes, and need to look into memory to register promotion next.

Interesting. Feel free to send me an email to bounce of ideas :)
You should also enable the Attributor once you start adding attributes ;)

I've made the changes, but I'm a little confused why read-only and write-only calls need to be handled differently.

Because we use the fact that something was written for reasoning, at least in globalopt. If the global was always written before it was read we basically privatize it in the function. We cannot ensure that it is actually written for write-only calls. Read only doesn't matter because we don't care if it may or must happen. Does that make sense? (Btw. I'm not an expert on this but I just read the surrounding code so I might be wrong.)

Why can't I assume that in the worst-case, the entire type is accessed?

The worst-case is fine but for a call it is: the entire type is read and something is written.

Also, what are the semantics of readonly and writeonly with respect to pointer casts? Isn't it valid behavior in C to cast any pointer type to void * as long as it is casted back to the original type before being accessed, so wouldn't this affect both loads and stores (that the actual load/store size could be larger than the argument type size)?

C doesn't really matter here but what you say is not wrong. We cannot conclude anything from the type of the pointer that goes into a call. That is why I said we have to assume the entire array is accessed. However,

// Assume that in the worst case, the entire type is accessed
Loads.push_back({CB, U->getType()->getPointerElementType()});

is not it. This uses the type at the call site. The entire thing works because we see the allocation so we know how big the entire thing is. Use the allocation type instead please.
(FWIW, even in C there is no need to cast it back into "the original type" in a lot of situations, including but not limited to access via char*.)

llvm/lib/Transforms/Utils/GlobalStatus.cpp
186

Are we sure Stored means "maybe stored"?

ddcc added a comment.Mar 28 2020, 3:31 PM

Thanks for the feedback, I've sent you an email.

Oops, I missed that writeonly is may-writeonly, which would break the store dominator analysis for the localize optimization. Should be fixed now.

ddcc updated this revision to Diff 253379.Mar 28 2020, 3:31 PM

Fix load type and store

ddcc updated this revision to Diff 253381.Mar 28 2020, 3:40 PM

Remove unneeded store code

ddcc updated this revision to Diff 253671.Mar 30 2020, 12:54 PM

Update tests with update_test_checks.py