Page MenuHomePhabricator

GlobalISel: Add generic instructions for memory intrinsics
ClosedPublic

Authored by arsenm on Aug 4 2020, 5:35 AM.

Details

Summary

AArch64, X86 and Mips currently directly consumes these and custom
lowering to produce a libcall, but really these should follow the
normal legalization process through the libcall/lower action.

Diff Detail

Event Timeline

arsenm created this revision.Aug 4 2020, 5:35 AM
arsenm requested review of this revision.Aug 4 2020, 5:35 AM

Would it be possible to add support for clampScalar for the size_t argument of these opcodes? Since there is still the problem in the frontend where these intrinsics are created with an incorrect type for the size_t argument, I think that would be useful.
Obviously it would be best to never encounter this case in the backend, but I don't see D76283 landing any time soon, so...

arsenm added a comment.Aug 4 2020, 6:04 AM

Would it be possible to add support for clampScalar for the size_t argument of these opcodes? Since there is still the problem in the frontend where these intrinsics are created with an incorrect type for the size_t argument, I think that would be useful.
Obviously it would be best to never encounter this case in the backend, but I don't see D76283 landing any time soon, so...

It could, but it isn't strictly necessary. The promotion gets inserted in the libcall lowering (and load/store introduced by the future lower action) so it ends up not mattering. It would be more useful if there was a target that had a native memcpy instruction of some kind

The promotion gets inserted in the libcall lowering [...]

What promotion do you mean? In its current from, createMemLibcall doesn't do any type promotion?

The problem we have is that some LLVM-IR passes introduce calls to llvm.memcpy.p0i8.p0i8.i64, even though our native pointer size is 32-bits. Since it turned out that fixing this in LLVM is quite invasive, we currently work around this problem by truncating the size argument to 32-bits before calling createMemLibcall.
Having the legalizer action do this for us, we could remove the custom handling from our backend.

arsenm added a comment.Aug 4 2020, 7:59 AM

The promotion gets inserted in the libcall lowering [...]

What promotion do you mean? In its current from, createMemLibcall doesn't do any type promotion?

If the argument size is narrower than the register it's passed it, it ends up inserting an extension to the register by virtue of the regular call lowering code

This patch just maintains the functionality that's there already, a new action to narrow the size is a separate patch

arsenm added a comment.Aug 4 2020, 8:03 AM

The problem we have is that some LLVM-IR passes introduce calls to llvm.memcpy.p0i8.p0i8.i64, even though our native pointer size is 32-bits. Since it turned out that fixing this in LLVM is quite invasive, we currently work around this problem by truncating the size argument to 32-bits before calling createMemLibcall.

You would have to deal with this whether passes introduce this IR or not, unless we make it a verifier error to deviate from the pointer size (which would be inconsistent with every other pointer sized integer operand, although I think this is pointless)

The problem we have is that some LLVM-IR passes introduce calls to llvm.memcpy.p0i8.p0i8.i64, even though our native pointer size is 32-bits. Since it turned out that fixing this in LLVM is quite invasive, we currently work around this problem by truncating the size argument to 32-bits before calling createMemLibcall.

You would have to deal with this whether passes introduce this IR or not, unless we make it a verifier error to deviate from the pointer size (which would be inconsistent with every other pointer sized integer operand, although I think this is pointless)

I'm thinking the IRTranslator should be responsible for this clamp, and the machine verifier should enforce the size of the length type. I'm not sure why we don't do this for G_INTTOPTR/G_PTRTOINT; I feel like the fact these aren't required to match the pointer size is a holdover from when the DataLayout was optional

The problem we have is that some LLVM-IR passes introduce calls to llvm.memcpy.p0i8.p0i8.i64, even though our native pointer size is 32-bits. Since it turned out that fixing this in LLVM is quite invasive, we currently work around this problem by truncating the size argument to 32-bits before calling createMemLibcall.

You would have to deal with this whether passes introduce this IR or not, unless we make it a verifier error to deviate from the pointer size (which would be inconsistent with every other pointer sized integer operand, although I think this is pointless)

I'm thinking the IRTranslator should be responsible for this clamp, and the machine verifier should enforce the size of the length type. I'm not sure why we don't do this for G_INTTOPTR/G_PTRTOINT; I feel like the fact these aren't required to match the pointer size is a holdover from when the DataLayout was optional

Having the IRTranslator perform the clamp would solve the problem for us, and then the verifier can also enforce the property, as you said. We have a similar rule in our LegalizerInfo for G_INTTOPTR/G_PTRTOINT to clamp to our pointer size. Again making this a verifier error could simplify our rules.

arsenm updated this revision to Diff 285987.Aug 17 2020, 5:43 AM

Cast to minimum in IRTranslator. I decided making this a verifier requirement is too strong since some target may have an instruction that looks like memcpy with a different integer size

arsenm updated this revision to Diff 285988.Aug 17 2020, 5:44 AM

Remove leftover comment

Cast to minimum in IRTranslator. I decided making this a verifier requirement is too strong since some target may have an instruction that looks like memcpy with a different integer size

Thank you for this! I think this is a good approach for this problem and we should be able to get rid of the legalizer workaround we have in place now.

aemerson accepted this revision.Aug 26 2020, 3:54 PM

LGTM.

This revision is now accepted and ready to land.Aug 26 2020, 3:54 PM

Hi Matt,

This was causing some test issues on the PS4 bots (e.g.
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/34314/)
as they only built X86 target.

I added this fix for the tests:
https://reviews.llvm.org/rG0b7f6cc71a72a85f8a0cbee836a7a8e31876951a

Please can you check you're happy with that?

Thanks
Russ

Hi Matt,

This was causing some test issues on the PS4 bots (e.g.
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/34314/)
as they only built X86 target.

I added this fix for the tests:
https://reviews.llvm.org/rG0b7f6cc71a72a85f8a0cbee836a7a8e31876951a

Please can you check you're happy with that?

Thanks
Russ

That's the right fix

That's the right fix

Thanks.

I noticed that some of the global-isel tests in this directory specify requires global-isel but some don't (e.g llvm/test/MachineVerifier/test_g_build_vector.mir) even though they specify -global-isel. Should this requirement be specified, or is this always available now?

That's the right fix

Thanks.

I noticed that some of the global-isel tests in this directory specify requires global-isel but some don't (e.g llvm/test/MachineVerifier/test_g_build_vector.mir) even though they specify -global-isel. Should this requirement be specified, or is this always available now?

It's always available now. The requires global-isel is a leftover that should be removed

It's always available now. The requires global-isel is a leftover that should be removed

Okay, thanks. I've opened https://reviews.llvm.org/D86714 to do that.