This is an archive of the discontinued LLVM Phabricator instance.

Lower arith.min/max to llvm.intr.minimum/maximum
Needs RevisionPublic

Authored by gflegar on Nov 10 2022, 7:03 AM.

Details

Summary

The current lowering was converting it into llvm.intr.minnum/maxnum which has
incompattible NaN behavior to the one specified by the arit.min/max ops.

Depends On D137655

Diff Detail

Event Timeline

gflegar created this revision.Nov 10 2022, 7:03 AM
Herald added a project: Restricted Project. · View Herald Transcript
gflegar requested review of this revision.Nov 10 2022, 7:03 AM
kuhar accepted this revision.Dec 19 2022, 7:36 AM

Could you update the patch description with a brief description of the new/intended behavior? This way it would be easier to tell what the change entails without cross-checking with the llvm language reference.

incompattible NaN behavior to the one specified by the arit.min/max ops.

nit: typo in 'arith'

This revision is now accepted and ready to land.Dec 19 2022, 7:36 AM

Should someone (maybe me) go ahead and land this?

This patch is depending on another one

arsenm requested changes to this revision.Jan 6 2023, 4:17 PM
arsenm added a subscriber: arsenm.

I don't think we even have a legalization implementation to expand these. You almost certainly intended to use llvm.minnum/maxnum

This revision now requires changes to proceed.Jan 6 2023, 4:17 PM

I don't think we even have a legalization implementation to expand these. You almost certainly intended to use llvm.minnum/maxnum

No, I did want to use llvm.minimum/maximum, because those are the instructions that have the correct semantics for arith.min/max.
We can't expand those in LLVM now, but that's what the dependent patch is for, however it needs more work to correctly expand it in a way that handles +/-0.0.
Don't have the cycles to figure that out at the moment though, so we can put this patch on hold for now (unless someone else wants to pick it up).

This should also answer @krzysz00's question: we can't land before the dependent patch is figured out.