[PATCH] D90247: [AArch64] Add legalizations for VECREDUCE_SEQ_FADD

Tue Oct 27 10:45:12 PDT 2020

cameron.mcinally added a comment.

In D90247#2357013 <https://reviews.llvm.org/D90247#2357013>, @nikic wrote:

>> A good example of this can be seen in @test_v3f32 from vecreduce-fadd-legalization-strict.ll. Here we end up with 4 FADDs, instead of the 3 FADDs required. The newly added FADD is the result of widening the illegal v3f32 vector type to v4f32, where the newly added element in the reduction is the "neutral" value, 0.0.
>
> Looking at https://github.com/llvm/llvm-project/blob/5a3ef55a524bf9e072d98286e5febdb218b1fc72/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L7477-L7480, shouldn't this just be a matter of using `-0.0` as the neutral element instead? If `0.0` is not actually neutral here, then this is not just suboptimal, it's incorrect. (We should fix this for the non-sequential case as well.)

@spatel

Yup, good catch. The (-0.0+0.0 -> +0.0) case is a problem. The identity should be -0.0.

>> And I'm still missing legalization support for some other actions, like SoftenFloat and PromoteFloat. I did not find existing tests for those actions.
>
> Copying llvm/test/CodeGen/ARM/vecreduce-fadd-legalization-soft-float.ll without the `fast` fmf should work as test coverage for that.

Ok, I can build that out. Are we okay with the suboptimal legalization though? I'll wait for that decision before putting more time into this.

Or does anyone see a clever fix for the illegal type legalization? It looks like we lost information during widening, so I'm not sure we can get it back in a non-hacky way.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90247