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.
Details
Diff Detail
Event Timeline
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.
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
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.
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.
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
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