Details
- Reviewers
paquette aemerson arsenm - Commits
- rGa64287247633: [GISel] Support llvm.memcpy.inline
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/MachineVerifier/test_g_memcpy.mir | ||
---|---|---|
16 | delete me |
llvm/include/llvm/IR/IntrinsicInst.h | ||
---|---|---|
895 ↗ | (On Diff #355232) | Looks like this is used in a few passes too (e.g. the MemCpyOptimizer pass). Does this change impact those passes at all? |
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
1357 | Probably worth a comment for folks not familiar with the code. It'd probably clearer to be able to call a inlineMemCpy function here and just return true, but I'm not sure if that's easy right now. | |
llvm/lib/CodeGen/MachineVerifier.cpp | ||
1510 | I think we only expect 0 or 1 for these values, so maybe we should check the value too? |
llvm/test/CodeGen/AArch64/GlobalISel/debug-loc-legalize-tail-call.mir | ||
---|---|---|
39 ↗ | (On Diff #355232) | If only memcpy has an inline variant then we shouldn't be generating G_MEMSETs with this extra argument? |
llvm/test/CodeGen/AArch64/GlobalISel/debug-loc-legalize-tail-call.mir | ||
---|---|---|
39 ↗ | (On Diff #355232) | I was on the fence about whether to let them all have the same number of arguments or not, since there are several places where code is shared between different opcodes and having them have different operand counts would lead to lots of special case logic. To strike a balance, maybe it should only be memcpy+memmove instead? |
llvm/test/CodeGen/AArch64/GlobalISel/debug-loc-legalize-tail-call.mir | ||
---|---|---|
39 ↗ | (On Diff #355232) | Why add to memmove though? Another way to implement this is to just introduce a new G_MEMCPY_INLINE opcode, then we don't have to add another argument anywhere. |
llvm/include/llvm/IR/IntrinsicInst.h | ||
---|---|---|
895 ↗ | (On Diff #355232) | Yes, this should be its own patch. I started doing this a few months ago but forgot about it |
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
1357 | Probably should use uint64_t? | |
llvm/test/CodeGen/AArch64/GlobalISel/debug-loc-legalize-tail-call.mir | ||
39 ↗ | (On Diff #355232) | I also think this should be a separate opcode. There's also the difference that memcpy.inline uses an immarg size argument (which I think is entirely pointless and should be removed, but that's a different battle) |
Make a new opcode: G_MEMCPY_INLINE, instead of adding extra arguments to existing opcodes.
Why change the existing memcpy test? I would have thought the logical way to test this was to use a G_MEMCPY_INLINE with a length much larger than the heuristics would normally allow for G_MEMCPY expansion, rather than change existing test coverage.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp | ||
---|---|---|
1347 | Dynamically sized memcpy.inline isn't legal according to the langref, so we should probably assert here. | |
llvm/test/CodeGen/AArch64/GlobalISel/inline-memcpy-opt.mir | ||
51 ↗ | (On Diff #355417) | This IR section is probably unnecessary. |
I'll rename the two tests to make it clear the existing one wasn't changed, but what happened is (roughly):
cp inline-memcpy.mir inline-memcpy-opt.mir sed -e 's/G_MEMCPY/G_MEMCPY_INLINE/' -e 's/memcpy\./memcpy.inline/g' inline-memcpy.mir update_mir_test_checks.py inline-memcpy.mir
llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir | ||
---|---|---|
632 | In the future, please use a regex to match any number instead of hard coding a number in here as it causes the test to fail if a downstream copy has private changes which affect which number is assigned to this opcode. I've fixed this up in f737d9794a40c066c9ccffb9ac277c1b70442ede. |
llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir | ||
---|---|---|
632 | Sorry about that. Thank you! |
Dynamically sized memcpy.inline isn't legal according to the langref, so we should probably assert here.