-
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
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: None (Shoreshen) ChangesPropagate alignment through ptrmask based on potential constant values of mask and align of ptr. Full diff: https://github.com/llvm/llvm-project/pull/150158.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index 3c24d2eca647d..67aed0a327f8e 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -5203,6 +5203,32 @@ static unsigned getKnownAlignForUse(Attributor &A, AAAlign &QueryingAA,
TrackUse = true;
return 0;
}
+ if (auto *II = dyn_cast<IntrinsicInst>(I)) {
+ if (II->getIntrinsicID() == Intrinsic::ptrmask) {
+ auto *ConstVals = A.getAAFor<AAPotentialConstantValues>(
+ QueryingAA, IRPosition::value(*(II->getOperand(1))),
+ DepClassTy::NONE);
+ const AAAlign *AlignAA = A.getAAFor<AAAlign>(
+ QueryingAA, IRPosition::value(*(II)), DepClassTy::NONE);
+ if (ConstVals && ConstVals->isValidState()) {
+ if (ConstVals->isAtFixpoint()) {
+ uint64_t TrailingZeros = 64;
+ for (const auto &It : ConstVals->getAssumedSet())
+ if (It.countTrailingZeros() < TrailingZeros)
+ TrailingZeros = It.countTrailingZeros();
+ if (TrailingZeros < 64) {
+ uint64_t Mask = 1 << TrailingZeros;
+ if (Mask >= AlignAA->getKnownAlign().value()) {
+ return 0;
+ }
+ }
+ return AlignAA->getKnownAlign().value();
+ }
+ } else if (AlignAA) {
+ return AlignAA->getKnownAlign().value();
+ }
+ }
+ }
MaybeAlign MA;
if (const auto *CB = dyn_cast<CallBase>(I)) {
@@ -5502,6 +5528,42 @@ struct AAAlignCallSiteReturned final
AAAlignCallSiteReturned(const IRPosition &IRP, Attributor &A)
: Base(IRP, A) {}
+ ChangeStatus updateImpl(Attributor &A) override {
+ Instruction *I = getIRPosition().getCtxI();
+ if (auto *II = dyn_cast<IntrinsicInst>(I)) {
+ if (II->getIntrinsicID() == Intrinsic::ptrmask) {
+ const AAPotentialConstantValues *ConstVals =
+ A.getAAFor<AAPotentialConstantValues>(
+ *this, IRPosition::value(*(II->getOperand(1))),
+ DepClassTy::REQUIRED);
+ const AAAlign *AlignAA =
+ A.getAAFor<AAAlign>(*this, IRPosition::value(*(II->getOperand(0))),
+ DepClassTy::REQUIRED);
+ uint64_t Alignment = 0;
+ if (ConstVals && ConstVals->isValidState()) {
+ unsigned TrailingZeros = 64;
+ for (const auto &It : ConstVals->getAssumedSet())
+ if (It.countTrailingZeros() < TrailingZeros)
+ TrailingZeros = It.countTrailingZeros();
+ if (TrailingZeros < 64)
+ Alignment = 1 << TrailingZeros;
+ }
+ if (AlignAA && AlignAA->isValidState() &&
+ Alignment < AlignAA->getAssumedAlign().value())
+ Alignment = AlignAA->getAssumedAlign().value();
+
+ if (Alignment != 0) {
+ uint64_t OldAssumed = getAssumed();
+ takeAssumedMinimum(Alignment);
+ return OldAssumed == getAssumed() ? ChangeStatus::UNCHANGED
+ : ChangeStatus::CHANGED;
+ } else {
+ return ChangeStatus::UNCHANGED;
+ }
+ }
+ }
+ return Base::updateImpl(A);
+ };
/// See AbstractAttribute::trackStatistics()
void trackStatistics() const override { STATS_DECLTRACK_CS_ATTR(align); }
};
diff --git a/llvm/test/Transforms/Attributor/align-ptrmask.ll b/llvm/test/Transforms/Attributor/align-ptrmask.ll
new file mode 100644
index 0000000000000..710f1c3983b7b
--- /dev/null
+++ b/llvm/test/Transforms/Attributor/align-ptrmask.ll
@@ -0,0 +1,101 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=attributor -S < %s | FileCheck %s
+
+define float @align_ptrmask_back_no_prop(ptr align 2 %x, i1 %cmp1, i1 %cmp2) {
+; CHECK-LABEL: define float @align_ptrmask_back_no_prop(
+; CHECK-SAME: ptr nofree readonly align 2 [[X:%.*]], i1 [[CMP1:%.*]], i1 [[CMP2:%.*]]) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP1]], i64 -32, i64 -8
+; CHECK-NEXT: [[SEL1:%.*]] = select i1 [[CMP2]], i64 [[SEL]], i64 -16
+; CHECK-NEXT: [[P:%.*]] = tail call align 8 ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 noundef [[SEL1]]) #[[ATTR2:[0-9]+]]
+; CHECK-NEXT: [[RES:%.*]] = load float, ptr [[P]], align 8, !invariant.load [[META0:![0-9]+]]
+; CHECK-NEXT: ret float [[RES]]
+;
+ %sel = select i1 %cmp1, i64 -32, i64 -8
+ %sel1 = select i1 %cmp2, i64 %sel, i64 -16
+ %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1)
+ %res = load float, ptr %p, align 8
+ ret float %res
+}
+
+define float @align_ptrmask_back_prop(ptr align 2 %x, i1 %cmp1, i1 %cmp2) {
+; CHECK-LABEL: define float @align_ptrmask_back_prop(
+; CHECK-SAME: ptr nofree readonly align 16 [[X:%.*]], i1 [[CMP1:%.*]], i1 [[CMP2:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP1]], i64 -32, i64 -8
+; CHECK-NEXT: [[SEL1:%.*]] = select i1 [[CMP2]], i64 [[SEL]], i64 -16
+; CHECK-NEXT: [[P:%.*]] = tail call align 16 ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 noundef [[SEL1]]) #[[ATTR2]]
+; CHECK-NEXT: [[RES:%.*]] = load float, ptr [[P]], align 16, !invariant.load [[META0]]
+; CHECK-NEXT: ret float [[RES]]
+;
+ %sel = select i1 %cmp1, i64 -32, i64 -8
+ %sel1 = select i1 %cmp2, i64 %sel, i64 -16
+ %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1)
+ %res = load float, ptr %p, align 16
+ ret float %res
+}
+
+define float @align_ptrmask_forward_mask(ptr align 2 %x, i1 %cmp1, i1 %cmp2) {
+; CHECK-LABEL: define float @align_ptrmask_forward_mask(
+; CHECK-SAME: ptr nofree readonly align 2 [[X:%.*]], i1 [[CMP1:%.*]], i1 [[CMP2:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP1]], i64 -32, i64 -8
+; CHECK-NEXT: [[SEL1:%.*]] = select i1 [[CMP2]], i64 [[SEL]], i64 -16
+; CHECK-NEXT: [[P:%.*]] = tail call align 8 ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 noundef [[SEL1]]) #[[ATTR2]]
+; CHECK-NEXT: [[RES:%.*]] = load float, ptr [[P]], align 8, !invariant.load [[META0]]
+; CHECK-NEXT: ret float [[RES]]
+;
+ %sel = select i1 %cmp1, i64 -32, i64 -8
+ %sel1 = select i1 %cmp2, i64 %sel, i64 -16
+ %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1)
+ %res = load float, ptr %p, align 4
+ ret float %res
+}
+
+define float @align_ptrmask_forward_ptr(ptr align 16 %x, i1 %cmp1, i1 %cmp2) {
+; CHECK-LABEL: define float @align_ptrmask_forward_ptr(
+; CHECK-SAME: ptr nofree readonly align 16 [[X:%.*]], i1 [[CMP1:%.*]], i1 [[CMP2:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP1]], i64 -32, i64 -8
+; CHECK-NEXT: [[SEL1:%.*]] = select i1 [[CMP2]], i64 [[SEL]], i64 -16
+; CHECK-NEXT: [[P:%.*]] = tail call align 16 ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 noundef [[SEL1]]) #[[ATTR2]]
+; CHECK-NEXT: [[RES:%.*]] = load float, ptr [[P]], align 16, !invariant.load [[META0]]
+; CHECK-NEXT: ret float [[RES]]
+;
+ %sel = select i1 %cmp1, i64 -32, i64 -8
+ %sel1 = select i1 %cmp2, i64 %sel, i64 -16
+ %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1)
+ %res = load float, ptr %p, align 4
+ ret float %res
+}
+
+define float @align_ptrmask_forward_nonconst_mask(ptr align 8 %x, i64 %y, i1 %cmp1, i1 %cmp2) {
+; CHECK-LABEL: define float @align_ptrmask_forward_nonconst_mask(
+; CHECK-SAME: ptr nofree readonly align 8 [[X:%.*]], i64 [[Y:%.*]], i1 [[CMP1:%.*]], i1 [[CMP2:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP1]], i64 -32, i64 [[Y]]
+; CHECK-NEXT: [[SEL1:%.*]] = select i1 [[CMP2]], i64 [[SEL]], i64 -16
+; CHECK-NEXT: [[P:%.*]] = tail call align 8 ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 [[SEL1]]) #[[ATTR2]]
+; CHECK-NEXT: [[RES:%.*]] = load float, ptr [[P]], align 8, !invariant.load [[META0]]
+; CHECK-NEXT: ret float [[RES]]
+;
+ %sel = select i1 %cmp1, i64 -32, i64 %y
+ %sel1 = select i1 %cmp2, i64 %sel, i64 -16
+ %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1)
+ %res = load float, ptr %p, align 4
+ ret float %res
+}
+
+define float @align_ptrmask_back_nonconst_mask(ptr align 4 %x, i64 %y, i1 %cmp1, i1 %cmp2) {
+; CHECK-LABEL: define float @align_ptrmask_back_nonconst_mask(
+; CHECK-SAME: ptr nofree readonly align 8 [[X:%.*]], i64 [[Y:%.*]], i1 [[CMP1:%.*]], i1 [[CMP2:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP1]], i64 -32, i64 [[Y]]
+; CHECK-NEXT: [[SEL1:%.*]] = select i1 [[CMP2]], i64 [[SEL]], i64 -16
+; CHECK-NEXT: [[P:%.*]] = tail call align 8 ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 [[SEL1]]) #[[ATTR2]]
+; CHECK-NEXT: [[RES:%.*]] = load float, ptr [[P]], align 8, !invariant.load [[META0]]
+; CHECK-NEXT: ret float [[RES]]
+;
+ %sel = select i1 %cmp1, i64 -32, i64 %y
+ %sel1 = select i1 %cmp2, i64 %sel, i64 -16
+ %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1)
+ %res = load float, ptr %p, align 8
+ ret float %res
+}
+;.
+; CHECK: [[META0]] = !{}
+;.
|
| return 0; | ||
| } | ||
| if (auto *II = dyn_cast<IntrinsicInst>(I)) { | ||
| if (II->getIntrinsicID() == Intrinsic::ptrmask) { |
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.
Can you split the intrinsic case into a separate function, and switch over the intrinsic ID
| DepClassTy::REQUIRED); | ||
| uint64_t Alignment = 0; | ||
| if (ConstVals && ConstVals->isValidState()) { | ||
| unsigned TrailingZeros = 64; |
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.
Take the pointer size instead of assuming 64
| takeAssumedMinimum(Alignment); | ||
| return OldAssumed == getAssumed() ? ChangeStatus::UNCHANGED | ||
| : ChangeStatus::CHANGED; | ||
| } else { |
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.
No else after return
| ; | ||
| %sel = select i1 %cmp1, i64 -32, i64 %y | ||
| %sel1 = select i1 %cmp2, i64 %sel, i64 -16 | ||
| %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1) |
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.
| %p = tail call ptr @llvm.ptrmask(ptr %x, i64 %sel1) | |
| %p = tail call ptr @llvm.ptrmask.p0.i64(ptr %x, i64 %sel1) |
Throughout
| %res = load float, ptr %p, align 8 | ||
| ret float %res | ||
| } | ||
| ;. |
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.
Test cases with constant values, which is the common case. Also test some unknowable cases. Also vectors
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 , I'm not sure if I can find a vector case. The current frame work seems stop it from generating AAAlign attribute for vector case:


Also extractelement cannot propagate alignment, so I also cannot use store/load to test
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.
extractelement can propagate alignment, whether or not it does currently is another question
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 , yep, sorry for how I said it. The current framework seems not supporting propagate alignment through extractelement. Would you like me to add simple support in this PR in order to add vector tests??
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.
No, add the vector test that shows it doesn't propagate for later improvement
| %res = load float, ptr %p, align 4 | ||
| ret float %res |
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.
Why not return the pointer value to see the inferred align on the return?
| uint64_t TrailingZeros = 64; | ||
| for (const auto &It : ConstVals->getAssumedSet()) | ||
| if (It.countTrailingZeros() < TrailingZeros) | ||
| TrailingZeros = It.countTrailingZeros(); | ||
| if (TrailingZeros < 64) { |
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.
There's no reason to have a limit of 64 or hardcode it, just directly use the size from the pointer
|
|
||
| namespace { | ||
|
|
||
| static std::optional<uint64_t> |
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 should keep this operating with Align instead of optional<uint64_t>
| unsigned Size = | ||
| DL.getPointerTypeSizeInBits(II->getOperand(0)->getType()); | ||
| uint64_t TrailingZeros = Size; | ||
| for (const auto &It : ConstVals->getAssumedSet()) |
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.
No aut o
| return Alignment; | ||
| } | ||
| return QueryingAA.getAssumedAlign().value(); | ||
| break; |
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.
Dead break
| return std::nullopt; | ||
| } | ||
|
|
||
| static std::optional<uint64_t> |
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.
Keep values in Align
| if (Alignment != 0) { | ||
| return Alignment; | ||
| } |
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.
| if (Alignment != 0) { | |
| return Alignment; | |
| } | |
| if (Alignment != 0) | |
| return Alignment; |
| const IntrinsicInst *II) { | ||
| switch (II->getIntrinsicID()) { | ||
| case Intrinsic::ptrmask: { | ||
| const AAPotentialConstantValues *ConstVals = |
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.
nit: in case particular case and the one below, you can use const auto *, because the template argument can tell what the type is.
| auto *ConstVals = A.getAAFor<AAPotentialConstantValues>( | ||
| QueryingAA, IRPosition::value(*(II->getOperand(1))), DepClassTy::NONE); | ||
| const AAAlign *AlignAA = A.getAAFor<AAAlign>( | ||
| QueryingAA, IRPosition::value(*(II)), DepClassTy::NONE); |
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.
keep consistent here as well
|
|
||
| namespace { | ||
|
|
||
| static std::optional<Align> getKnownAlignForIntrinsic(Attributor &A, |
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.
We have a dedicated MaybeAlign, but this shouldn't be optional at all (imo MaybeAlign should be removed). Align(1) is always a valid value, the logic should be based on Align > Known
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 , for other intrinsic hasn't be handled inside the function, it will return nullopt for now.
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 always use Align(1) as a maximally conservative result
| if (It.countTrailingZeros() < TrailingZeros) | ||
| TrailingZeros = It.countTrailingZeros(); | ||
| if (TrailingZeros < Size) { | ||
| uint64_t Mask = 1 << TrailingZeros; |
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.
Keep this in APInt to avoid UB for wide pointers
| const auto *AlignAA = | ||
| A.getAAFor<AAAlign>(QueryingAA, IRPosition::value(*(II->getOperand(0))), | ||
| DepClassTy::REQUIRED); | ||
| uint64_t Alignment = 0; |
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.
Use Align
|
|
||
| namespace { | ||
|
|
||
| Align getAssumedAlignForIntrinsic(Attributor &A, AAAlign &QueryingAA, |
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.
| 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.
| return clampStateAndIndicateChange<StateType>(this->getState(), | ||
| Align.value()); | ||
| } | ||
| return Base::updateImpl(A); |
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.
| DepClassTy::REQUIRED); | ||
| if (ConstVals && ConstVals->isValidState()) { | ||
| unsigned ShiftValue = | ||
| std::min(ConstVals->getAssumedMinTrailingZeros(), 63U); |
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.
LG
|
|
||
| namespace { | ||
|
|
||
| static Align getAssumedAlignForIntrinsic(Attributor &A, AAAlign &QueryingAA, |
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.
Since the known one gets inlined, this one can probably be 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 can't comment on the code itself, but I can say that the tests give the impression of producing expected results
| unsigned ShiftValue = std::min(ConstVals->getAssumedMinTrailingZeros(), | ||
| Value::MaxAlignmentExponent); | ||
| Align ConstAlign(UINT64_C(1) << ShiftValue); | ||
| if (ConstAlign >= AlignAA->getKnownAlign()) |
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
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/15230 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/12550 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/12357 Here is the relevant piece of the build log for the reference |
Propagate alignment through ptrmask based on potential constant values of mask and align of ptr.