This is an archive of the discontinued LLVM Phabricator instance.

[flang] Support late math lowering for intrinsics from the llvm table.
ClosedPublic

Authored by vzakhari on Jul 18 2022, 4:01 PM.

Details

Summary

mathOpearations should now support all intrinsics that are handled
by the llvmIntrinsics table + tan lowered as Math dialect operation +
f128 flavor of abs.

I am going to flip the default to late math lowering after this change,
but still keep the fallback via pgmath. This will allow getting rid
of the llvmIntrinsics table and continue populating
only the mathOperations table, otherwise, updating both tables
seems to be inconvenient.

Diff Detail

Event Timeline

vzakhari created this revision.Jul 18 2022, 4:01 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 18 2022, 4:01 PM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
vzakhari requested review of this revision.Jul 18 2022, 4:01 PM
This revision is now accepted and ready to land.Jul 19 2022, 3:02 AM

From the summary:

[flang] Support late math lowering for intrinsics from the llvm table.

This doesn't seem right - isn't this patch about adding the missing intrisincs into mathOperations and llvmIntrinsics tables? And making sure that both tables are consistent (hence the new entry for f128 flavor of abs). Otherwise looks good, thanks!

[nit] "mathOpearations" --> "mathOperations"

From the summary:

[flang] Support late math lowering for intrinsics from the llvm table.

This doesn't seem right - isn't this patch about adding the missing intrisincs into mathOperations and llvmIntrinsics tables? And making sure that both tables are consistent (hence the new entry for f128 flavor of abs). Otherwise looks good, thanks!

[nit] "mathOpearations" --> "mathOperations"

Yes, the patch is about making the two tables consistent + adding a couple of new intrinsics. Since the removal of llvmIntrinsics will come next I tried to keep the two tables consistent, hence the addition of f128 abs to llvmIntrinsics. The addition was not strictly necessary, but I decided to keep them consistent until the actual removal commit. tan is added only to mathOperations, because llvmIntrinsics cannot easily handle math::TanOp.

Yes, the patch is about making the two tables consistent + adding a couple of new intrinsics. Since the removal of llvmIntrinsics will come next I tried to keep the two tables consistent, hence the addition of f128 abs to llvmIntrinsics. The addition was not strictly necessary, but I decided to keep them consistent until the actual removal commit. tan is added only to mathOperations, because llvmIntrinsics cannot easily handle math::TanOp.

This is fine, but your summary refers to "the llvm table" . What is this "llvm table"? llvmIntrinsics? This is not clear from the summary. Also, most changes are in fact in mathOperations. I appreciate that this is clear for you, but for me this is a bit out of context and a bit more precise summary would really help. I'm also being mindful of our future selves when the context is going to be even less clear, or people who haven't been following this work at all, i.e. stuff from https://reviews.llvm.org/D128385. Having a Discourse thread that could be referred to (or any sort of design discussion) would also make things easier. Once that's in place, you could label this as "Late math intrinsics lowering Step 2/n".

I mostly wanted to clarify the source of confusion.

Yes, the patch is about making the two tables consistent + adding a couple of new intrinsics. Since the removal of llvmIntrinsics will come next I tried to keep the two tables consistent, hence the addition of f128 abs to llvmIntrinsics. The addition was not strictly necessary, but I decided to keep them consistent until the actual removal commit. tan is added only to mathOperations, because llvmIntrinsics cannot easily handle math::TanOp.

This is fine, but your summary refers to "the llvm table" . What is this "llvm table"? llvmIntrinsics? This is not clear from the summary. Also, most changes are in fact in mathOperations. I appreciate that this is clear for you, but for me this is a bit out of context and a bit more precise summary would really help. I'm also being mindful of our future selves when the context is going to be even less clear, or people who haven't been following this work at all, i.e. stuff from https://reviews.llvm.org/D128385. Having a Discourse thread that could be referred to (or any sort of design discussion) would also make things easier. Once that's in place, you could label this as "Late math intrinsics lowering Step 2/n".

I mostly wanted to clarify the source of confusion.

Thank you for the explanation, @awarzynski. I cannot say that I can outline all the steps right now, but I started a topic, that can hold all the major decisions/questions-answers: https://discourse.llvm.org/t/rfc-change-lowering-of-fortran-math-intrinsics/63971