Page MenuHomePhabricator

GlobalISel: Merge G_PTR_MASK with llvm.ptrmask intrinsic
ClosedPublic

Authored by arsenm on May 15 2020, 6:23 PM.

Details

Summary

Confusingly, these were unrelated and had different semantics. The
G_PTR_MASK instruction predates the llvm.ptrmask intrinsic, but has a
different format. G_PTR_MASK only allows clearing the low bits of a
pointer, and only a constant number of bits. The ptrmask intrinsic
allows an arbitrary mask. Replace G_PTR_MASK to match the intrinsic.

Only selects the cases that look like the old instruction. More work
is needed to select the general case. Also new legalization code is
still needed to deal with the case where the incoming mask size does
not match the pointer size, which has a specified behavior in the
langref.

Diff Detail

Event Timeline

arsenm created this revision.May 15 2020, 6:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2020, 6:23 PM
gargaroff accepted this revision.May 17 2020, 11:11 PM

LGTM with a nit. Thanks for unifying the semantics!

llvm/test/MachineVerifier/test_g_ptrmask.mir
19

Should the verifier report where the type mismatch is? It already reports if the destination of the mask is incorrect. Shouldn't it do the same for the source?

This revision is now accepted and ready to land.May 17 2020, 11:11 PM
arsenm marked an inline comment as done.May 18 2020, 3:02 PM
arsenm added inline comments.
llvm/test/MachineVerifier/test_g_ptrmask.mir
19

Generally the verifier seems to just try every single check independently and produce every error it can, even if it's redundant

dsanders added inline comments.May 19 2020, 7:14 PM
llvm/include/llvm/Support/TargetOpcodes.def
554

Did you mean to drop the underscore? It's inconsistent with G_PTR_ADD now

llvm/lib/CodeGen/MachineVerifier.cpp
1105–1120

Could you add these requirements to GenericOpcodes.rst (and correct the semantics in that description too)?

arsenm marked an inline comment as done.May 19 2020, 7:22 PM
arsenm added inline comments.
llvm/include/llvm/Support/TargetOpcodes.def
554

Yes. It's now consistent with the IR intrinsic

arsenm closed this revision.May 26 2020, 8:48 AM
arsenm marked 2 inline comments as done.
llvm/include/llvm/Support/TargetOpcodes.def
554

Really I'd rather rename the IR intrinsic, but it's probably not worth the hassle