This is an archive of the discontinued LLVM Phabricator instance.

[GISel] Support llvm.memcpy.inline
ClosedPublic

Authored by jroelofs on Jun 28 2021, 6:14 PM.

Diff Detail

Event Timeline

jroelofs created this revision.Jun 28 2021, 6:14 PM
jroelofs requested review of this revision.Jun 28 2021, 6:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 6:14 PM
jroelofs added inline comments.Jun 28 2021, 6:18 PM
llvm/test/MachineVerifier/test_g_memcpy.mir
16

delete me

jroelofs updated this revision to Diff 355232.Jun 29 2021, 7:35 AM
  • clang-format the patch
  • Delete stray declaration
  • Add IRTranslator test
paquette added inline comments.Jun 29 2021, 10:42 AM
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?

jroelofs added inline comments.Jun 29 2021, 10:51 AM
llvm/include/llvm/IR/IntrinsicInst.h
895 ↗(On Diff #355232)

Yes. Maybe I ought to put that in its own diff.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1357

Good point. I'll give that a try.

llvm/lib/CodeGen/MachineVerifier.cpp
1510

Good call!

aemerson added inline comments.Jun 29 2021, 10:56 AM
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?

jroelofs added inline comments.Jun 29 2021, 11:01 AM
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?

aemerson added inline comments.Jun 29 2021, 11:31 AM
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.

arsenm added inline comments.Jun 29 2021, 1:22 PM
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)

jroelofs updated this revision to Diff 355417.Jun 29 2021, 6:15 PM

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.

jroelofs marked 2 inline comments as done.Jun 30 2021, 8:56 AM

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

Can you add an entry on G_MEMCPY_INLINE to docs/GlobalISel/GenericOpcode.rst?

jroelofs updated this revision to Diff 355610.Jun 30 2021, 9:58 AM
jroelofs marked an inline comment as done.Jun 30 2021, 10:02 AM
This revision is now accepted and ready to land.Jun 30 2021, 11:16 AM
This revision was landed with ongoing or failed builds.Jun 30 2021, 12:39 PM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Jul 1 2021, 11:12 PM
dyung added inline comments.
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.

jroelofs added inline comments.Jul 2 2021, 8:42 AM
llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir
632

Sorry about that. Thank you!