[PATCH] D76638: [SDAG] fix crash in getNegatedExpression() by ignoring transient fadd

Fri Apr 3 05:52:27 PDT 2020

spatel added a comment.

> ! In D76638#1959075 <https://reviews.llvm.org/D76638#1959075>, @steven.zhang wrote:
>  Yeah, it is sensitive to how we combine the nodes. I tried your patch(add FENG check for FADD), and see two extra instructions generated as you mentioned,  on X86. So, we need some positive tests for this patch if it helps the codegen. What do you think ?

Yes, I see that this patch would regress that case - the 2 instructions are the extra fadd/fsub and a move - but it still gives the slight improvement on the 1st test (fsub becomes fadd).
I'm going to step through the new code with a few examples to get a better idea of the possibilities.

If it's not necessary to add complexity, then I'll abandon this patch. It's a question of how often would we realistically expect to see non-canonical patterns by the time we reach here.

The (fadd (fneg X), Y) pattern should not occur in optimized IR (instcombine should fold it), so as long as we're not crashing on it, we are probably ok. It would be a nice enhancement to -reassociate and/or -instcombine to optimize the negated common term from the equation in IR.

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

https://reviews.llvm.org/D76638