Page MenuHomePhabricator

[ValueTracking] Add tracking of the alignment assume bundle
ClosedPublic

Authored by scui on Oct 1 2020, 9:13 AM.

Details

Summary

This patch is to add the support of the value tracking of the alignment assume bundle.

Diff Detail

Event Timeline

scui created this revision.Oct 1 2020, 9:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 9:13 AM
scui requested review of this revision.Oct 1 2020, 9:13 AM

Look OK to me but you should also add other reviewers.

scui added a reviewer: nikic.Oct 2 2020, 11:14 AM
nikic edited reviewers, added: Tyker, jdoerfert, efriedma; removed: eli.friedman.Oct 2 2020, 1:23 PM
scui added a comment.Oct 13 2020, 8:58 AM

Anyone has any comments for this patch? Any comments or suggestions are appreciated.

What does the third operand of an align bundle mean? It doesn't seem to be documented in LangRef.

jdoerfert added inline comments.Oct 13 2020, 2:02 PM
llvm/lib/Analysis/ValueTracking.cpp
735

You don't want to look for the bundle yourself. Use

SmallVector<Attribute::AttrKind, 2> AttrKinds{Attribute::Align};
if (RetainedKnowledge RK = getKnowledgeValidInContext(V, AttrKinds, CtxI, DT, AC)) {
  ...
}

instead to account for all the interesting cases, e.g., multiple align bundles. Also, use braces for non trivial conditionals. I don't think the return is what you want here, other deductions might still apply. Also, I think we have alignment bundle -> Align information in the AlignmentFromAssumptions.cpp pass. We should create a helper function so this is less magic.

scui added a comment.EditedOct 13 2020, 2:06 PM

What does the third operand of an align bundle mean? It doesn't seem to be documented in LangRef.

You are right. My understanding is that align bundle of (p, 32, 24) assumes (p+24) is 32-byte aligned. - typo corrected.

What does the third operand of an align bundle mean? It doesn't seem to be documented in LangRef.

You are right. My understanding is that align bundle of (p, 32, 24) assumes (p-24) is 32-byte aligned.

That's correct. It matches the old llvm.assume alignment encoding via instructions.
I mentioned before that we should probably just create a gep with the offset instead of keeping it as a third operand but as long as we do we should have a helper to extract it from the bundle.

scui updated this revision to Diff 298169.Oct 14 2020, 9:54 AM

Rewrite the code using getKnowledgeValidInContext. The code is much cleaner than the previous version, but the result computed is less accurate if the size of the alignment bundle is three. I'll leave the improvement for the future, together with the improvement of value tracking of GEP (an example is that, with the future improvement, we can fold away the branch "br i1 %3, ...") in f2 of the added case assume-align.ll).

scui added inline comments.Oct 14 2020, 9:58 AM
llvm/lib/Analysis/ValueTracking.cpp
735

You don't want to look for the bundle yourself. Use

SmallVector<Attribute::AttrKind, 2> AttrKinds{Attribute::Align};
if (RetainedKnowledge RK = getKnowledgeValidInContext(V, AttrKinds, CtxI, DT, AC)) {
  ...
}

instead to account for all the interesting cases, e.g., multiple align bundles. Also, use braces for non trivial conditionals. I don't think the return is what you want here, other deductions might still apply. Also, I think we have alignment bundle -> Align information in the AlignmentFromAssumptions.cpp pass. We should create a helper function so this is less magic.

Thanks for the comments and suggestions. I have rewrite the code using getKnowledgeValidInContext. Please take a look. Thanks

jdoerfert accepted this revision.Oct 14 2020, 11:46 AM

Some nits you should address, otherwise LGTM.

Rewrite the code using getKnowledgeValidInContext. The code is much cleaner than the previous version, but the result computed is less accurate if the size of the alignment bundle is three. I'll leave the improvement for the future, together with the improvement of value tracking of GEP (an example is that, with the future improvement, we can fold away the branch "br i1 %3, ...") in f2 of the added case assume-align.ll).

FWIW, we should either not encode the offset but use a GEP, or make getKnowledgeValidInContext aware. Either should be a follow up.

llvm/lib/Analysis/ValueTracking.cpp
681

I don't think the parenthesis part in the comment should be here.

llvm/test/Transforms/InstCombine/assume-align.ll
49

Instead of the comment above, add a TODO/FIXME here describing the two potential solutions to this problem.

This revision is now accepted and ready to land.Oct 14 2020, 11:46 AM

Reverse ping. More comments (nits) and notice that we should merge this, D89830 reimplemented it in the meantime, see my comment there too.

llvm/lib/Analysis/ValueTracking.cpp
676

No need for the vector, replace it below with {Attribute::Alignment}.

In D89830 @arichardson uses Log2_32 instead of countTrailingZeros. I think that makes sense since we have a power of 2.

scui updated this revision to Diff 299946.Oct 22 2020, 6:25 AM

Updated to address review comments: I adopted the use of Log2_32 from D89830, removed the parenthesis comment, and added a TODO comment in the test case.

scui added a comment.Oct 22 2020, 6:28 AM

Reverse ping. More comments (nits) and notice that we should merge this, D89830 reimplemented it in the meantime, see my comment there too.

Thanks for the comments. I don’t have commit access. If we commit this first, I will need your help to commit for me (“Shimin Cui <scui@ca.ibm.com>”). Otherwise @arichardson can merge this with D89830 to commit both.