tensorflow / mlir

"Multi-Level Intermediate Representation" Compiler Infrastructure
1.73k stars 257 forks source link

Add OpenMP dialect to the dialect registry #244

Closed DavidTruby closed 4 years ago

DavidTruby commented 4 years ago

This dialect will be used by flang for OpenMP code generation

googlebot commented 4 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

DavidTruby commented 4 years ago

Arm has signed the CLA.

@googlebot I signed it!

googlebot commented 4 years ago

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

joker-eph commented 4 years ago

Just to confirm: is this dialect gonna be "abstract" or "generic" OpenMP or is it gonna be "Fortran Specific" OpenMP? (for instance you may let things like types creep in). If the later then I rather see it reflected in the name.

joker-eph commented 4 years ago

Just to confirm: is this dialect gonna be "abstract" or "generic" OpenMP or is it gonna be "Fortran Specific" OpenMP? (for instance you may let things like types creep in). If the later then I rather see it reflected in the name.

Also: if this is a generic OpenMP dialect and not Fortran specific, I would expect it to be submitted upstream and not live in the flang tree? Can you clarify your plans about this?

DavidTruby commented 4 years ago

The current plan is to make this a generic OpenMP IR without Fortran specific features as we would like to leave open the possibility for clang to use this if(/when?) they move to an mlir-based code generator. We are also currently planning to submit this upstream in mlir and not in the flang tree.

It's possible of course that when we start implementing this we might find we need Fortran specific things anyway, but the plan is to avoid anything Fortran specific if at all possible.

joker-eph commented 4 years ago

Great! Thanks, I'd encourage you to send an RFC with the dialect design and the first "milestone" you'd like to reach: it'll likely smooth out patch review :)

DavidTruby commented 4 years ago

I'd encourage you to send an RFC with the dialect design and the first "milestone" you'd like to reach: it'll likely smooth out patch review :)

Most of the discussion on dialect design has been happening in the flang community. We have some design walkthroughs, I'll see if we can get some cross-conversation going here with the MLIR community.

I have also been preparing a patch to add an OpenMP dialect and a single operation omp.barrier so that we can start work on the dialect. This probably qualifies as the first milestone we're aiming for :)

joker-eph commented 4 years ago

Most of the discussion on dialect design has been happening in the flang community. We have some design walkthroughs, I'll see if we can get some cross-conversation going here with the MLIR community.

Thanks! We will definitely need something like this before reviewing patches I think

nicolasvasilache commented 4 years ago

I'd be quite interested in also seeing what the end to end path to execution looks like.

It would be great if some execution flow could be made available early in the MLIR repo to experiment.

On Tue, Nov 19, 2019, 4:48 PM Mehdi Amini notifications@github.com wrote:

Most of the discussion on dialect design has been happening in the flang community. We have some design walkthroughs, I'll see if we can get some cross-conversation going here with the MLIR community.

Thanks! We will definitely need something like this before reviewing patches I think

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/mlir/pull/244?email_source=notifications&email_token=ACNNU5E53QAXPTRGUU7WQJDQURNL5A5CNFSM4JOV4762YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEP4RDY#issuecomment-555731087, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNNU5GP2TI6Z2JWO6BZ7ALQURNL5ANCNFSM4JOV476Q .

DavidTruby commented 4 years ago

Is this waiting on more design info to be merged or can we get this initial registration in before then?

joker-eph commented 4 years ago

Is this waiting on more design info to be merged or can we get this initial registration in before then?

No this is all good, for some reason our integration script failed and I didn't notice. I restarted it and hopefully it should go through! Sorry for the delay.