Page MenuHomePhabricator

IR: Merge MemCpyInlineInst and MemCpyInst
Changes PlannedPublic

Authored by arsenm on Aug 17 2020, 5:42 AM.

Details

Summary

llvm.memcpy.inline does not deviate from llvm.memcpy in any way that's
meaningful to the IR. This avoids writing more code in a future
commit. Not sure why lint handled these slightly differently.

Diff Detail

Event Timeline

arsenm created this revision.Aug 17 2020, 5:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 5:42 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
arsenm requested review of this revision.Aug 17 2020, 5:42 AM
arsenm updated this revision to Diff 285998.Aug 17 2020, 6:15 AM
arsenm planned changes to this revision.Aug 17 2020, 6:16 AM

Still requires updating more places that create new memcpys from existing ones to clean up this mess

gchatelet added a subscriber: jfb.EditedSep 11 2020, 1:03 AM

Still requires updating more places that create new memcpys from existing ones to clean up this mess

Originally llvm.memcpy.inline has been created as a separate IR instruction to allow for the semantic to diverge if needed.
Now in the light of D79279 it seems that we're heading towards many subtlety different IR memory functions, this comes with maintenance cost and complexity.

Maybe we should keep only one that has all the expressive power and lower to it.

I'm summing up the envisioned properties for memcpy here (mostly the ones from D79279)

  • Source and destination pointers:
    • must be of any trivially copyable types
    • may come with alignment guarantees
    • may be tagged as volatile or provide atomic access semantic
    • may be from different pointer spaces? <- not sure about this one (edit: @arsenm I'm watching your talk about address spaces, it seems some of the requirements can be encoded with this notion)
  • A constant force_inline argument can instruct the compiler to generate the whole content inline, in that case size must be a ConstantInt, if this argument is false, the compiler may choose to delegate implementation to an external function that has the same semantic (libc or user provided).

The same exercise would be needed for memset and memcmp.

@jfb what do you think? I know your patch is almost ready so I'd rather have this discussion before it's submitted.
Maybe this conversation should take place on the dev list even?

Having a bunch of boolean modifiers floating around on memcpy sounds miserable for code to deal with; it makes it hard to code to just ignore the "complicated" memcpy operations if it doesn't want to reason about them. (It's already messy just dealing with volatile operations.)

llvm/lib/Analysis/Lint.cpp
343

IIRC the reason it was implemented this way is that the length passed to memcpy_inline is marked immarg, so we can be more aggressive. I guess the paths can be merged, though.