[PATCH] Gut InstCombiner::SimplifyMemTransfer.

Tue Aug 18 17:01:51 PDT 2015

On Tue, Aug 18, 2015 at 4:57 PM, John McCall <rjmccall at apple.com> wrote:

>
> > On Aug 16, 2015, at 3:08 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> >
> > [+Richard, John, Chandler]
> >
> > ----- Original Message -----
> >> From: "David Majnemer via llvm-commits" <llvm-commits at lists.llvm.org>
> >> To: "Lang Hames" <lhames at gmail.com>
> >> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at lists.llvm.org
> >
> >> Sent: Wednesday, August 12, 2015 10:35:16 PM
> >> Subject: Re: [PATCH] Gut InstCombiner::SimplifyMemTransfer.
> >>
> >>
> >>
> >> Hi Lang,
> >>
> >>
> >> I reviewed the InstCombine side and it looks great, all that I ask is
> >> that you replace the `Instruction *` with `auto *` because the type
> >> is replicated in the `dyn_cast`.
> >>
> >>
> >> My only concern is that clang will actually create calls to the
> >> memcpy intrinsic from within clang with pointers which exactly
> >> alias:
> >> http://clang.llvm.org/doxygen/CGExprAgg_8cpp_source.html#l01448
> >>
> >
> > That seems very broken. We need to fix this. In fact, the comment admits
> as much:
> >
> > 01454   // memcpy is not defined if the source and destination pointers
> are exactly
> > 01455   // equal, but other compilers do this optimization, and almost
> every memcpy
> > 01456   // implementation handles this case safely.  If there is a libc
> that does not
> > 01457   // safely handle this, we can add a target hook.
> >
> > Is there any reason not to emit memmove instead?
>
> Partial overlaps are generally not possible here — C guarantees that the
> overlap
> in an assignment has to be perfect.  So a great deal of the added
> complexity of
> memmove is not needed; and as mentioned, no actual implementations of
> memcpy misbehave for perfect overlap.
>
> That said, I can appreciate wanting to keep the formal model sane.  I
> suspect that
> emitting llvm.memmove would generally produce uses of the actual memmove,
> but I'd be happy to use a different intrinsic that allows only perfect
> overlap and which
> you can make a “platform-specific” decision to lower to memcpy in the
> backend.
>

I suggested to Lang on IRC that we might want to add another operand to the
memcpy intrinsic to model this behavior.  Another intrinsic would, of
course, work just as well.  AFAICT, the only code which would care about
the distinction between memcpy as-written-in-source and
as-written-by-compiler which would make the extra flag a little less
disruptive than another intrinsic.

>
> John.
>
> >
> > -Hal
> >
> >>
> >> I cannot confess to being an alias-analysis expert but my concern is
> >> that annotating the memcpy as such could create some interesting
> >> paradoxes. Those more familiar with AA should chime in.
> >>
> >>
> >> I'm afraid I cannot provide any guidance on the SelectionDAG changes.
> >>
> >>
> >> Thanks
> >> --
> >> David Majnemer
> >>
> >>
> >> On Wed, Aug 12, 2015 at 10:06 PM, Lang Hames via llvm-commits <
> >> llvm-commits at lists.llvm.org > wrote:
> >>
> >>
> >>
> >> Another belated *ping*.
> >>
> >>
> >>
> >> - Lang.
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> On Tue, Jun 30, 2015 at 2:16 PM, Lang Hames < lhames at gmail.com >
> >> wrote:
> >>
> >>
> >>
> >> Belated ping. :)
> >>
> >>
> >> - Lang.
> >>
> >>
> >>
> >>
> >> On Tue, May 12, 2015 at 2:11 PM, Lang Hames < lhames at gmail.com >
> >> wrote:
> >>
> >>
> >>
> >> Gentle ping.
> >>
> >>
> >> - Lang.
> >>
> >>
> >>
> >>
> >> On Tue, Apr 28, 2015 at 11:51 AM, Lang Hames < lhames at gmail.com >
> >> wrote:
> >>
> >>
> >>
> >> Ping / delayed reply.
> >>
> >>
> >> DannyB - thanks for the extra context. :)
> >>
> >>
> >> Chandler - any thoughts? Assuming you still want to keep this
> >> transform, what do you think of my instcombine change? (i.e. tagging
> >> the load/store pointers as non-aliasing for memcpy).
> >>
> >>
> >> Cheers,
> >> Lang.
> >>
> >>
> >>
> >>
> >>
> >>
> >> On Tue, Apr 21, 2015 at 9:21 PM, Daniel Berlin < dberlin at dberlin.org
> >>> wrote:
> >>
> >>
> >> On Wed, Apr 8, 2015 at 10:38 PM, Chandler Carruth <
> >> chandlerc at gmail.com > wrote:
> >>> On Wed, Apr 8, 2015 at 10:15 PM Lang Hames < lhames at gmail.com >
> >>> wrote:
> >>>>
> >>>> Hi David, Chandler,
> >>>>
> >>>> The attached patch guts the SimplifyMemTransfer method, which
> >>>> turns small
> >>>> memcpys (1/2/4/8 bytes) into load/store pairs. Turning memcpys
> >>>> into
> >>>> load/store pairs loses information, since we can no longer assume
> >>>> the source
> >>>> and dest are non-overlapping. This is leading to some suboptimal
> >>>> expansions
> >>>> for small memcpys on AArch64 when -mno-unaligned-access is turned
> >>>> on (see
> >>>> r234462). I suspect other architectures would suffer similar
> >>>> issues.
> >>>>
> >>>> I assume this transform is an old workaround to simplify other
> >>>> non-memcpy-aware IR transforms. These days I think most IR
> >>>> transforms can
> >>>> reason sensibly about memcpys, so I'm hoping this is safe to
> >>>> remove. FWIW,
> >>>> removing it didn't hit any regression tests except those that were
> >>>> verifying
> >>>> that this optimisation was being applied, but then you wouldn't
> >>>> really
> >>>> expect it to hit any others.
> >>>
> >>>
> >>> Heh. I tried to remove it before and it regressed a *lot* of
> >>> performance.
> >>> Have you measured it? I think there are many places that don't
> >>> today reason
> >>> about memcpy but do reason about loads and stores. Here is a
> >>> partial list:
> >>>
> >>> - GVN
> >>
> >> Note:
> >>
> >> It reasons about memory intrinsics, including memcpy and memset, and
> >> will pull values out to forward.
> >>
> >> for memset, if the load is in the memset size, it will compute what
> >> the value is.
> >> For memcpy, it handles anything that is constant memory.
> >>
> >>
> >> (Certainly, loads and stores are handled better, but ...)
> >>
> >>
> >>
> >>
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150818/e782ad04/attachment.html>