This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] FNMUL
ClosedPublic

Authored by SjoerdMeijer on Nov 14 2022, 12:39 AM.

Details

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Nov 14 2022, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 12:39 AM
SjoerdMeijer requested review of this revision.Nov 14 2022, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 12:39 AM

Can we please more verbose title for the patch?

llvm/test/CodeGen/AArch64/fnmul.ll
54

The test cases cover -x * y test cases. Can we have x * -y too? I believe this patch will handle those cases too. However, it is good to have a test case.

Added the tests.

I think this subject + body is an excellent description of the things here:

[AArch64] FNMUL
This adds match patterns for the reassociated forms of FNMUL.

I could merge this all into the subject, have no body, but I don't think this is all very important for this simple patch...

NoHonorSignDependentRounding is the old way of doing things - it should be based on fastmath flags nowadays. Or at least I've not heard of NoHonorSignDependentRounding before and it seems to be only used in the Arm backend at the moment.

This alive test seems to suggest that we can just always do the transform: https://alive2.llvm.org/ce/z/5-LdY1. Which I think sounds OK considering that it is a fneg. Do you know why HonorSignDependentRounding is needed? Was it just copied from Arm?

SjoerdMeijer added a comment.EditedNov 14 2022, 4:31 AM

NoHonorSignDependentRounding is the old way of doing things - it should be based on fastmath flags nowadays. Or at least I've not heard of NoHonorSignDependentRounding before and it seems to be only used in the Arm backend at the moment.

This alive test seems to suggest that we can just always do the transform: https://alive2.llvm.org/ce/z/5-LdY1. Which I think sounds OK considering that it is a fneg. Do you know why HonorSignDependentRounding is needed? Was it just copied from Arm?

Thanks Dave.
Yeah, I also think we don't need NoHonorSignDependentRounding, but just added it to be consistent with ARM. The default is off, so it will do the transform, which is why I didn't mind adding it... but you're right it's probably better not to add it if it is the old way of doing things. So I will remove it from here. Let's then also remove it for ARM, but we can do that separately.

  • removed NoHonorSignDependentRounding from the rules
  • changed tests to run with and without fp-contract=fast, and some cases with and without fast IR instruction to show it should always trigger.
dmgreen accepted this revision.Nov 14 2022, 5:37 AM

Thanks. Sounds good. LGTM

This revision is now accepted and ready to land.Nov 14 2022, 5:37 AM
This revision was landed with ongoing or failed builds.Nov 14 2022, 6:34 AM
This revision was automatically updated to reflect the committed changes.