Page MenuHomePhabricator

[AArch64] Add FCMLA AArch64ISD node.
Needs ReviewPublic

Authored by fhahn on Nov 12 2020, 6:27 AM.

Event Timeline

fhahn created this revision.Nov 12 2020, 6:27 AM
fhahn requested review of this revision.Nov 12 2020, 6:27 AM

Sounds OK, but I think there's such a thing as splitting up a patch too much! And if it's not possible to add tests for something, that can be a bad sign.

Complex MLA is something that exists in MVE too. I'm not sure what the other part of this looks like yet (I presume it's just matching), but it may be good in the long run to make some of this more target independent, so long as they work in the same way.

fhahn added a comment.Nov 12 2020, 9:06 AM

Sounds OK, but I think there's such a thing as splitting up a patch too much! And if it's not possible to add tests for something, that can be a bad sign.

That's a good point! The patch that's using it is D91354, but it relies on a new intrinsic which is still up in the air. I post on llvm-dev to restart the discussion about how to improve support for complex math: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146568.html

Complex MLA is something that exists in MVE too. I'm not sure what the other part of this looks like yet (I presume it's just matching), but it may be good in the long run to make some of this more target independent, so long as they work in the same way.

If there are very similar instructions in other backends, it might make sense to introduce an independent ISD node in the future, but I am only familiar with the complex math instructions on AArch64 unfortunately.

OK sure. I was expecting some ISel lowering, to be honest. And perhaps for a vplan patch to appear :)

At a concrete level, matching in CodeGenPrep doesn't sound ideal, unless we expect these to spill over multiple basic blocks a lot of the time. At the moment we could get the same effect by matching in ISel, like any other instruction.

If like I suspect (/hope :)) the goal is to pattern match during vectorization and produce something better there - relying on a "complex multply" intrinsic may not be optimal in terms of the patterns you can recognize. The VCMLA and VCMUL/VCADD operations are more general than that and can match other patterns. Things like conjugates and rotates can modify that. I can see how that would be harder to make look target independent though.

fhahn updated this revision to Diff 376840.Oct 4 2021, 3:07 AM

rebased, still WIP.

pengfei added a subscriber: pengfei.Oct 4 2021, 5:39 AM
Matt added a subscriber: Matt.Oct 5 2021, 12:46 PM

Doesn't make much sense to me having this on it's own... merging with D91354 seems like the sensible choice.