llvm dev - Dec 2020 - [GlobalISel] Legalization for memcpy family intrinsics

> On Dec 16, 2020, at 13:52, Amara Emerson via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
>> On Dec 16, 2020, at 6:47 AM, Devadasan, Christudasan
<Christudasan.Devadasan at amd.com <mailto:Christudasan.Devadasan at
amd.com>> wrote:
>> 
>> Hi All,
>>  
>> I was trying to legalize the family of memcpy intrinsics (memcpy,
memmove and memset) for AMDGPU and found that the combiner functions
optimizeMemcpy, optimizeMemmove and optimizeMemset implemented in
CombinerHelper.cpp pretty much handle these lowering.
>>  
>> Can we move these functions to Legalizer entirely and perform a custom
legalization to handle their expansion?
>> The reason is, in the Combiner it is more of an optimization action
rather than something required always for correctness.
> Right, that’s how this is implemented in SelectionDAG too, it’s an
optimization that may or may not fire depending on the heuristics.
This is a largely a function of SelectionDAG not supporting the necessary
loop-based expansions since you can’t introduce control flow. The current scheme
is a hacky split between IR and DAG expansions.
>>  
>> On the other hand, if these combiner expansions turned out to be
unavoidable, we should move these function into a common file.
>> In that way, both Legalizer and Combiner can take advantage of them.
> I think refactoring it to be shared is ok, but I and others disagree that
this is a legalization issue rather than a combiner. There is no question of
legality here, the target should be able to handle these opcodes. If they’re not
legal for your target, then you could simply not use the expansion combines and
handle them using custom legalization like any other operation
There is a question of legality, just as with every other opcode. The legalizer
should support the necessary expansion, and these shouldn’t be special.The
expansion can be done context free, so I think it fundamentally isn't a
combine. Splitting the expansion decisions between two places would be just as
messy as the DAG

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20201216/a49d17f3/attachment.html>

Can we conclude this discussion?
I believe legalizer is the right place for this expansion.

Regards,
CD

From: Matt Arsenault <whatmannerofburgeristhis at gmail.com> On Behalf Of
Matt Arsenault
Sent: Thursday, December 17, 2020 7:38 AM
To: Amara Emerson <amara at apple.com>
Cc: Devadasan, Christudasan <Christudasan.Devadasan at amd.com>; llvm-dev
at lists.llvm.org
Subject: Re: [llvm-dev] [GlobalISel] Legalization for memcpy family intrinsics

[CAUTION: External Email]



On Dec 16, 2020, at 13:52, Amara Emerson via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:



On Dec 16, 2020, at 6:47 AM, Devadasan, Christudasan <Christudasan.Devadasan
at amd.com<mailto:Christudasan.Devadasan at amd.com>> wrote:

Hi All,

I was trying to legalize the family of memcpy intrinsics (memcpy, memmove and
memset) for AMDGPU and found that the combiner functions optimizeMemcpy,
optimizeMemmove and optimizeMemset implemented in CombinerHelper.cpp pretty much
handle these lowering.

Can we move these functions to Legalizer entirely and perform a custom
legalization to handle their expansion?
The reason is, in the Combiner it is more of an optimization action rather than
something required always for correctness.
Right, that’s how this is implemented in SelectionDAG too, it’s an optimization
that may or may not fire depending on the heuristics.

This is a largely a function of SelectionDAG not supporting the necessary
loop-based expansions since you can’t introduce control flow. The current scheme
is a hacky split between IR and DAG expansions.


On the other hand, if these combiner expansions turned out to be unavoidable, we
should move these function into a common file.
In that way, both Legalizer and Combiner can take advantage of them.
I think refactoring it to be shared is ok, but I and others disagree that this
is a legalization issue rather than a combiner. There is no question of legality
here, the target should be able to handle these opcodes. If they’re not legal
for your target, then you could simply not use the expansion combines and handle
them using custom legalization like any other operation

There is a question of legality, just as with every other opcode. The legalizer
should support the necessary expansion, and these shouldn’t be special.The
expansion can be done context free, so I think it fundamentally isn't a
combine. Splitting the expansion decisions between two places would be just as
messy as the DAG

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20201221/d8a0ead6/attachment.html>

Pinging once again after the long holiday.

Regards,
CD

From: Devadasan, Christudasan
Sent: Monday, December 21, 2020 7:40 PM
To: Matt Arsenault <arsenm2 at gmail.com>; Amara Emerson <amara at
apple.com>
Cc: llvm-dev at lists.llvm.org
Subject: RE: [llvm-dev] [GlobalISel] Legalization for memcpy family intrinsics

Can we conclude this discussion?
I believe legalizer is the right place for this expansion.

Regards,
CD

From: Matt Arsenault <whatmannerofburgeristhis at
gmail.com<mailto:whatmannerofburgeristhis at gmail.com>> On Behalf Of
Matt Arsenault
Sent: Thursday, December 17, 2020 7:38 AM
To: Amara Emerson <amara at apple.com<mailto:amara at apple.com>>
Cc: Devadasan, Christudasan <Christudasan.Devadasan at
amd.com<mailto:Christudasan.Devadasan at amd.com>>; llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] [GlobalISel] Legalization for memcpy family intrinsics

[CAUTION: External Email]


On Dec 16, 2020, at 13:52, Amara Emerson via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:


On Dec 16, 2020, at 6:47 AM, Devadasan, Christudasan <Christudasan.Devadasan
at amd.com<mailto:Christudasan.Devadasan at amd.com>> wrote:

Hi All,

I was trying to legalize the family of memcpy intrinsics (memcpy, memmove and
memset) for AMDGPU and found that the combiner functions optimizeMemcpy,
optimizeMemmove and optimizeMemset implemented in CombinerHelper.cpp pretty much
handle these lowering.

Can we move these functions to Legalizer entirely and perform a custom
legalization to handle their expansion?
The reason is, in the Combiner it is more of an optimization action rather than
something required always for correctness.
Right, that’s how this is implemented in SelectionDAG too, it’s an optimization
that may or may not fire depending on the heuristics.

This is a largely a function of SelectionDAG not supporting the necessary
loop-based expansions since you can’t introduce control flow. The current scheme
is a hacky split between IR and DAG expansions.


On the other hand, if these combiner expansions turned out to be unavoidable, we
should move these function into a common file.
In that way, both Legalizer and Combiner can take advantage of them.
I think refactoring it to be shared is ok, but I and others disagree that this
is a legalization issue rather than a combiner. There is no question of legality
here, the target should be able to handle these opcodes. If they’re not legal
for your target, then you could simply not use the expansion combines and handle
them using custom legalization like any other operation

There is a question of legality, just as with every other opcode. The legalizer
should support the necessary expansion, and these shouldn’t be special.The
expansion can be done context free, so I think it fundamentally isn't a
combine. Splitting the expansion decisions between two places would be just as
messy as the DAG

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210111/40fb0dc9/attachment-0001.html>