-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[Attributor] Propagate alignment through ptrmask #150158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 81 commits
bd9c95b
eecf59e
3a161f3
cd46759
48cd7d6
8091ffb
7b1abf2
461f0b1
258238f
8878983
ec9dccd
420ab45
22ee90f
ebbfc0b
52b16d4
47dc434
4c8af0f
9aa5bdb
9985f51
794c7f0
3d8aaad
50a1bbd
3ee0bba
4b69c00
6241734
4613413
1aaa568
ac0f753
166cbf9
0105ebd
9efeb6f
5ae0e7f
2eeff68
801c6b6
736bd31
63cac5f
2213f73
91bc830
e833e8b
5b95504
a065a18
57b4b2b
5268447
7188f35
09549d9
9755380
2a53aa7
d2445fe
c8ddb15
1dc52e2
bce47af
65e7872
c6c5131
5a9099a
c34df24
9ffe9ef
4fed48b
62277ae
14e6de2
1190cff
db748bf
572e2b2
1432758
edf463a
a4b632b
cb48630
ff78488
62375b2
835bebe
0c5723a
ad59b3e
f930468
add9024
eb3b946
4716ce0
71c1e45
4db0555
7b4737a
7cf4cf5
558acc7
2020b77
15322ab
6937c0a
ad75dcc
c42ed04
91edd4e
fca2371
9fbc571
c5650f1
38cac2f
db99e2e
caf71d2
7ac364f
846e4db
0dc1ac3
37197f2
54e1e6c
8b023c0
bdf2ec6
cf9bc32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5187,6 +5187,33 @@ struct AADereferenceableCallSiteReturned final | |||||
| // ------------------------ Align Argument Attribute ------------------------ | ||||||
|
|
||||||
| namespace { | ||||||
|
|
||||||
| Align getAssumedAlignForIntrinsic(Attributor &A, AAAlign &QueryingAA, | ||||||
|
||||||
| Align getAssumedAlignForIntrinsic(Attributor &A, AAAlign &QueryingAA, | |
| static Align getAssumedAlignForIntrinsic(Attributor &A, AAAlign &QueryingAA, |
In fact, the latest code standard recommends using anonymous namespace as small as possible, and use static on internal functions instead, but that would be in a follow-up.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use Value::MaxAlignmentExponent here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might just not be understanding the attributor very well, but it seems weird that we're setting align 1 here. I assume it's because there's some handling later that fixes this case?
I can tell from the tests that all this code is doing what I'd intuitively expect, so I'll just let myself be confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @krzysz00 this means the mask is going to "cancel out" the alignment requirement, say if we have:
define internal ptr @test(ptr align 4 %x) {
%p = tail call ptr @llvm.ptrmask.p0.i64(ptr %x, i64 -16)
store float 1.0, ptr %p, align 8
ret ptr %p
}
And we are calculating the alignment of %x, since %p is masked by -16, it is aligned at 16. Then it doesn't matter what is the alignment of %x. So this use of %x returns 1, which means no alignment requirement of this use for %x. The final required alignment of %x depends on all of its use.
Otherwise, if we have:
define internal ptr @test(ptr align 4 %x) {
%p = tail call ptr @llvm.ptrmask.p0.i64(ptr %x, i64 -4)
store float 1.0, ptr %p, align 8
ret ptr %p
}
Then if %x is aligned at 4, it must not meet the requirement for the store instruction, so the alignment of %x should be at least 8. Same, for this use of %x it requires %x's alignment is at least 8
shiltian marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we want to simply give up here if it is an intrinsic call and getAssumedAlignForIntrinsic can't handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shiltian , same as known alignment I cannot tell from the return value that if it is correctly handled.
But what is different here is if it is intrinsic, the call will be equivalently handled as call to deceleration, which will create a pessimistic alignment attribute for the caller. so the following handle can be ignored and directly return here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can compare with Align(), no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shiltian , Align() is same as Align(1), the ShiftValue = 0 and the value() returns uint64_t(1) << ShiftValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm lost here. 1 << 0 is also Align(1) right? Align(1) is the worst alignment anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shiltian , Align(n) is giving an alignment of n , it will take log of the input:

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so confused. Inside getAssumedAlignForIntrinsic, if the intrinsic can't be handled, it simply returns Align(), no? That being said, instead of completely giving up when getAssumedAlignForIntrinsic can't handle, you can still check if getAssumedAlignForIntrinsic returns Align(). If it does, it can continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shiltian , if the return is Align, we cannot tell from the return value that if the intrinsic is handled.... I used to use std::optional<Align> but Matt said it should be Align. And it also confused me that Align()=Align(1), the default ShiftValue is 0, and if we take the log of 1, it's also 0.......
However if I'm not wrong, let it continue will simply return the alignment of the intrinsic. Since intrinsics are function declaration, so it will initialize but not update and directly indicate pessimistic and make the "assumed = known".
May be we can implement the logic inside getAssumedAlignForIntrinsic??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When it returns Align() (the one from default c'tor), it doesn't matter whether it is the intrinsic not handled, or it is already handled but just happens to be that, because Align() is the worst case anyway. It also doesn't matter whether the state is persimistic or optimistic if the intrinsic indeed is Align().
I understand for now it will not make any difference but I'd just like to be a little bit future proof here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why we need both of these functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @arsenm , one is for known and one is for assumed.
Known is the maximum alignment that is required from its use. For example:
Since
loadrequire%xto align at 8, the alignment of%xis at least 8.Assumed is the minimum alignment calculated from source. For example:
The alignment from
%pshould be 8 as calculated from%xand%y.The final alignment will be max of assumed and known.
Back to
%p = llvm.ptrmask(%x, %mask), the first is to calculate the known alignment for%x, the second is used to calculate the assumed alignment for%p. They have different functionalities.