[PATCH] D86468: [WIP][GlobalISel] CSE copies

Wed Aug 26 08:41:25 PDT 2020

foad added a comment.

In D86468#2238773 <https://reviews.llvm.org/D86468#2238773>, @arsenm wrote:

> In D86468#2238317 <https://reviews.llvm.org/D86468#2238317>, @foad wrote:
>
>> In D86468#2234049 <https://reviews.llvm.org/D86468#2234049>, @arsenm wrote:
>>
>>> I always thought all the stuff SelectionDAG::getNode tried to do was insane, and a large part of why the DAG is so slow.
>>
>> If you're ever going to do these simplifications, then surely it makes sense to do them up front so you don't have to create the MIR, rather than doing them in a later combine/simplification pass?
>>
>>> How often do these trivially simplifiable situations actually get produced during legalization and combines? I would expect it's pretty rare
>>
>> I tried a quick hack to simplify integer arithmetic in MIRBuilder, and you can see the effect on AMDGPU legalization tests here: https://github.com/jayfoad/llvm-project/commit/00d98a70c4dc8e8db2f8465104a5e65ca12409f5
>
> These look like some of the cases I want to have an optimizing combiner during the legalizer, but I'm not sure it means they should be done in the MIRBuilder. Some of these cases I think are avoidable. For example, I don't think we take the best path to legalize merge/unmerge, which in turn ends up producing more of these trivial situations

Yes we could avoid some trivial cases where we do a buildAdd of a buildConstant of 0 or similar: https://github.com/jayfoad/llvm-project/commit/90bc2d596adac8c2e47cc3c763a648da580feb92

But I think most of the simplification opportunities come from lowering/legalizing ops whose operands may or may not be constants, and the lowering/legalization code doesn't bother to check for the constant case. And personally I think it would be tedious and error-prone to have to do that, when you could rely on a builder to do it for you instead.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86468/new/

https://reviews.llvm.org/D86468