This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Canonicalize `getelementptr` patterns to `@llvm.ptrmask`
Needs ReviewPublic

Authored by goldstein.w.n on Jun 28 2023, 2:18 PM.

Details

Reviewers
nikic
RKSimon
Summary

[InstCombine] Fold getelementptr patterns to @llvm.ptrmask

@llvm.ptrmask is documented as being equivilent to:
(getelementptr i8 Ptr, (sub (and (ptrtoint Ptr), Mask), (ptrtoint Ptr)))

See: https://llvm.org/docs/LangRef.html#llvm-ptrmask-intrinsic

As well clang's __builtin_align_up and __builtin_align_down emit
patterns that can be folded to pointer mask.

Handle all three.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jun 28 2023, 2:18 PM
goldstein.w.n requested review of this revision.Jun 28 2023, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 2:18 PM
nikic requested changes to this revision.Jun 28 2023, 2:32 PM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2247

This is not legal, because it does not preserve pointer provenance. This one even fails to preserve it in a particularly catastrophic way, because the resulting pointer has nullary provenance (i.e. it is UB to perform any accesses through it).

This revision now requires changes to proceed.Jun 28 2023, 2:32 PM
goldstein.w.n added inline comments.Jun 28 2023, 2:44 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2247

I see, is the codes to create @llvm.ptrmask okay? Or should that too be abandoned?

nikic added inline comments.Jun 28 2023, 2:47 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2247

If you start at the documented GEP pattern (rather than the one based on null), then that should be okay.

goldstein.w.n added inline comments.Jun 28 2023, 2:48 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2247

Assuming I change the null version to be the sub + and version.

Update impl to only handle clear GEP -> ptrmask patterns

Would it also make sense to update the align_up codegen to use this intrinsic directly? I didn't do it at the time since there were concerns about using ptrmask, but it seems like optimization passes handle it now?

Would it also make sense to update the align_up codegen to use this intrinsic directly? I didn't do it at the time since there were concerns about using ptrmask, but it seems like optimization passes handle it now?

I think probably. What prompted this was the recent support added for it in knownbits + the fact that we do a better just assigning access attributes (and ingeneral tracking pointers) when there is no ptrtoint.

nikic added a comment.EditedJun 29 2023, 12:37 AM

Can you please add proofs for cases 2 and 3?

This generally looks fine to me, but I think we need slightly better ptrmask support before we land this. My minimum expectation would be ability to fold away ptrmask on already aligned pointers.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2249

Missing , after Ptr.

nikic added a comment.Jul 27 2023, 5:56 AM

FYI alive2 now has ptrmask support.

FYI alive2 now has ptrmask support.

Oh nice :)
I'll add proofs when I get to properly supporting ptrmask + rebasing this.