This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Docs] Provide implementation status details
ClosedPublic

Authored by jdoerfert on Jul 8 2019, 3:49 PM.

Event Timeline

jdoerfert created this revision.Jul 8 2019, 3:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 3:49 PM

The scheme itself looks good in general. Just as an improvement, it would be good to use coloring for the feature status.
Also, what's the difference between claimed, mostly done and done?

clang/docs/OpenMPSupport.rst
152

I'm working on this feature, but don't know when it's going to be ready.

218

Do we really need Fortran stuff here?

236

Not sure 100%, but seems to me it is not done.

jdoerfert marked 2 inline comments as done.Jul 8 2019, 4:21 PM

The scheme itself looks good in general.

Good. Once we agreed on a scheme we can improve the actual process, excel sheet, generator script, etc. (see also below).

Just as an improvement, it would be good to use coloring for the feature status.

I'll try to add coloring.

Also, what's the difference between claimed, mostly done and done?

Disclaimer: I used a script to determine what the columns in the excel document mean. There is a good chance I got some wrong and it only reflects the information in the document.

I basically used the following logic in this order:

  1. If all non-empty columns contain the word "done", mark it as "done".
  2. If all non-empty columns contain "mostly complete", mark it as "mostly done".
  3. If there are non-empty columns, mark it as "claimed", e.g., someone is working on it.
  4. If there are only empty columns, mark it as "unclaimed", e.g., not being worked on right now.

I think when we go through the excel document now with this use case in mind and fill the empty columns appropriately, some errors will dissapear.

clang/docs/OpenMPSupport.rst
152

That will then be noted in the excel sheet (see general comment).

218

No, I already filtered by hand, my mistake. Will automate it.

The scheme itself looks good in general.

Good. Once we agreed on a scheme we can improve the actual process, excel sheet, generator script, etc. (see also below).

Just as an improvement, it would be good to use coloring for the feature status.

I'll try to add coloring.

Also, what's the difference between claimed, mostly done and done?

Disclaimer: I used a script to determine what the columns in the excel document mean. There is a good chance I got some wrong and it only reflects the information in the document.

I basically used the following logic in this order:

  1. If all non-empty columns contain the word "done", mark it as "done".
  2. If all non-empty columns contain "mostly complete", mark it as "mostly done".
  3. If there are non-empty columns, mark it as "claimed", e.g., someone is working on it.
  4. If there are only empty columns, mark it as "unclaimed", e.g., not being worked on right now.

I think when we go through the excel document now with this use case in mind and fill the empty columns appropriately, some errors will dissapear.

Ok, I see. I will provide more correct feature status tomorrow, if you want. Currently it is not quite correct/full.

Nice, thank you for working on this!

It would be also good to have a not OpenMP-5 specific, but an overview table - which OpenMP thingy was supported starting
with which Clang version, very much like https://clang.llvm.org/cxx_status.html / https://libcxx.llvm.org/cxx1z_status.html,
i needed that a few times.

Hahnfeld added inline comments.
clang/docs/OpenMPSupport.rst
164

This is not correct, at least it's not yet upstream.

172

This is wrong as well, at least not upstream (we have multiple versions in local repositories).

ABataev added inline comments.Jul 9 2019, 7:10 AM
clang/docs/OpenMPSupport.rst
160

Done

162

Done

174–176

I don't remember anything related to these items. Seems to me, they are unclaimed

200

Seems to me it is unclaimed

204

Can't find this in the standard.

206

Done.

212

Done

214

Done

232

Done

242

This is just the runtime part, the compiler does not support this

244–246

Not sure that there are special requirements for this in the standard

248

lambdas are supported.

kkwli0 added inline comments.Jul 9 2019, 1:16 PM
clang/docs/OpenMPSupport.rst
204

Section 2.12.7

232

Do we support the behavior in 318:7-14?

236

I think we still need the codegen patch and I am not sure about the runtime part.

242

Since it is a hint according to the specification, I guess it is up to us whether we want to declare this feature done or not. If we do that, we should mention it in the limitation section.

ABataev added inline comments.Jul 9 2019, 1:27 PM
clang/docs/OpenMPSupport.rst
204

Then it is unclaimed, I think.

232

Yes.

236

I don't think it works with unified memory since we don't fully support unified memory.

242

Still, compiler does not use this. WE can mark this as partial, but definitely not done.

I will update the table (hopefully tomorrow) and we can then see if we commit it and change it in-place or if we have more initial feedback.

Thanks everyone for providing all this information!

@lebedev.ri I'd like to see if we can transition this one into a more generic one with version numbers etc. Is that OK?

@lebedev.ri I'd like to see if we can transition this one into a more generic one with version numbers etc. Is that OK?

I have no further comment at the moment.

jdoerfert updated this revision to Diff 209073.Jul 10 2019, 3:00 PM
jdoerfert marked 29 inline comments as done.

Add colors, updates according to comments

jdoerfert added inline comments.Jul 10 2019, 3:04 PM
clang/docs/OpenMPSupport.rst
162

Here and above, if there are more revisions, please feel free to add them.

164

Is there anything upstreamed? What should I put for status and revisions/reviews?

204

The excel sheet did assign some of the claimed tasks to people/companies so I will keep them claimed for now and we should ask if they are actually being worked on or not.

206

I only found a 2017 patch which I added but is this completely done? The category suggest this is device related but I did not find device related code.

kkwli0 added inline comments.Jul 10 2019, 3:39 PM
clang/docs/OpenMPSupport.rst
184

parallell -> parallel

188

parallell -> parallel

ABataev added inline comments.Jul 10 2019, 3:42 PM
clang/docs/OpenMPSupport.rst
206

Ah, I missed that this is device extension. Then it is unclaimed.

kkwli0 added inline comments.Jul 10 2019, 3:55 PM
clang/docs/OpenMPSupport.rst
198

Change OMP_TARGET_FALLBACK to OMP_TARGET_OFFLOAD which is in the spec.

jdoerfert updated this revision to Diff 209089.Jul 10 2019, 3:59 PM
jdoerfert marked 4 inline comments as done.

Fixes according to comments

Hahnfeld added inline comments.Jul 10 2019, 11:42 PM
clang/docs/OpenMPSupport.rst
164

I don't think there's a review yet. @protze.joachim ?

198

This is done, see the linked revision.

250

I think this should say partial, it's present in libomp after D55078.

I'll add @Hahnfeld comments, anything else? If not, can someone approve this and we do separate reviews for improvements?

I'll add @Hahnfeld comments, anything else? If not, can someone approve this and we do separate reviews for improvements?

Update revision and I'll approve it.

jdoerfert updated this revision to Diff 212957.Aug 1 2019, 7:54 PM

Improve based on two comments by @Hahnfeld

Hahnfeld added inline comments.Aug 1 2019, 11:54 PM
clang/docs/OpenMPSupport.rst
222–226

Do we have CodeGen for this? I only remember Sema support...

Also, reverse_offload is listed again below as unclaimed.

232
250

ping, I still think partial is more correct

jdenny added a subscriber: jdenny.Aug 6 2019, 2:56 PM
ABataev added inline comments.Aug 8 2019, 10:36 AM
clang/docs/OpenMPSupport.rst
222–226

We don't have codegen for this. Same for the reverse_offload and atomic_default_mem_order

clang/docs/OpenMPSupport.rst
222–226

As Alexey said codegen is not done so maybe change it to partial.
Regardng reverse_offload, it is debate able whether keep it unclaimed or make it partial. Yes Sema is done but the hard part for which nobody has claimed is codegen and libomptarget. At least for the getting this initial document checked in we should keep it as unclaimed. Do you have a strong objection against this.

jdoerfert updated this revision to Diff 217631.Aug 28 2019, 6:50 AM
jdoerfert marked 2 inline comments as done.

Update according to comments

ABataev added inline comments.Aug 28 2019, 6:58 AM
clang/docs/OpenMPSupport.rst
204

This feature is implemented already

jdoerfert marked an inline comment as done.Sep 4 2019, 8:56 AM
jdoerfert added inline comments.
clang/docs/OpenMPSupport.rst
204

If we wait longer, we can always update this patch. Maybe we should upstream it and update it online.

@RaviNarayanaswamy @kkwli0 @ABataev

ABataev accepted this revision.Sep 4 2019, 8:59 AM

LG

clang/docs/OpenMPSupport.rst
204

Add the latest info and commit it.

This revision is now accepted and ready to land.Sep 4 2019, 8:59 AM
kkwli0 added inline comments.Sep 4 2019, 9:03 AM
clang/docs/OpenMPSupport.rst
204

Agreed.

protze.joachim added inline comments.Sep 4 2019, 9:07 AM
clang/docs/OpenMPSupport.rst
164

There is no review yet. Implementation is available in
https://github.com/OpenMPToolsInterface/LLVM-openmp/tree/ompd-tests

This revision was automatically updated to reflect the committed changes.
jdoerfert marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 10:14 AM