Page MenuHomePhabricator

[OPENMP] Codegen for "omp flush" directive.
ClosedPublic

Authored by ABataev on Nov 17 2014, 1:20 AM.

Details

Summary

For each "omp flush" directive a call to "void kmpc_flush(ident_t *, ...)" function is generated.
Directive "omp flush" may have an associated list of variables to flush, but currently runtime function ignores them. So the patch generates just "call
kmpc_flush(ident_t *<loc>)".

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 16281.Nov 17 2014, 1:20 AM
ABataev retitled this revision from to [OPENMP] Codegen for "omp flush" directive..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).
rjmccall edited edge metadata.Nov 18 2014, 2:24 PM

One conceptual objection. Code-wise, seems fine.

lib/CodeGen/CGOpenMPRuntime.cpp
667 ↗(On Diff #16281)

To what extent does OpenMP have an ABI? Like, okay, I accept that the current OpenMP runtime does nothing with the list of variables. Do we not care that a future OpenMP runtime might want to do something? Are we implicitly hard-coding an assumption that the compiler will always ship with some exact version of the runtime?

The runtime spec even calls this out as being useless because the varargs aren't terminated. So why is this even specified as a variadic function?

Please fix this. At the very least, either:

  1. Accept that this runtime function will never try to do a more restricted flush, and therefore make this a non-varargs function,
  2. Specify some way to say how many variables there are. The best way to do this, for a runtime function where we don't really care about the convenience of the caller, is to pass the count as a non-varargs argument.

If you're doing #2 — and I think you probably should — and you don't want to do the work of materializing the addresses, then you should also specify that passing no addresses requests a full memory fence.

[+Jim]

  • Original Message -----

From: "John McCall" <rjmccall@gmail.com>
To: "a bataev" <a.bataev@hotmail.com>, fraggamuffin@gmail.com, ejstotzer@gmail.com, rjmccall@gmail.com
Cc: cfe-commits@cs.uiuc.edu
Sent: Tuesday, November 18, 2014 4:24:10 PM
Subject: Re: [PATCH] [OPENMP] Codegen for "omp flush" directive.

One conceptual objection. Code-wise, seems fine.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:667
@@ +666,3 @@
+ Build call void __kmpc_flush(ident_t *loc, ...)
+
List of variables is ignored by libiomp5 runtime, no need to
generate it.
+ llvm::Value *Args[] = {EmitOpenMPUpdateLocation(CGF, Loc)};


To what extent does OpenMP have an ABI? Like, okay, I accept that
the current OpenMP runtime does nothing with the list of variables.

Do we not care that a future OpenMP runtime might want to do

something? Are we implicitly hard-coding an assumption that the
compiler will always ship with some exact version of the runtime?

The runtime spec even calls this out as being useless because the
varargs aren't terminated. So why is this even specified as a
variadic function?

Please fix this. At the very least, either:

  1. Accept that this runtime function will never try to do a more restricted flush, and therefore make this a non-varargs function,
  2. Specify some way to say how many variables there are. The best way to do this, for a runtime function where we don't really care about the convenience of the caller, is to pass the count as a non-varargs argument.

    If you're doing #2 — and I think you probably should — and you don't want to do the work of materializing the addresses, then you should also specify that passing no addresses requests a full memory fence.

    http://reviews.llvm.org/D6292

cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

John, thanks for the review.

lib/CodeGen/CGOpenMPRuntime.cpp
667 ↗(On Diff #16281)

John, as a temporary solution I can add an int32 0 as a second parameter to this function call and add a comment that this must be fixed once the runtime function is changed and add a note that full mem fence is requested here.

ABataev updated this revision to Diff 16368.Nov 19 2014, 1:55 AM
ABataev edited edge metadata.

Added i32 0 argument to call of "__kmpc_flush()" function and changed a comment in lib codegen class.

Okay, that's fine by me. Please do make sure that this change is ultimately made to the runtime, though.

One small tweak, but with that, LGTM.

lib/CodeGen/CGOpenMPRuntime.cpp
670 ↗(On Diff #16368)

Prefer ConstantInt::get(CGM.Int32Ty, 0).

ABataev closed this revision.Nov 19 2014, 8:35 PM
ABataev updated this revision to Diff 16411.

Closed by commit rL222409 (authored by @ABataev).

From a runtime point of view we need to preserve backwards binary compatibility, so we can't change the interface to the current interface function to introduce a count (because that old code won't set it).

I would therefore prefer to do this

  1. Change the prototype for the existing function so that it becomes a function with no arguments. (Effectively your #1). Since no existing code passes arguments, this is fine.
  1. If/when we decide that it is useful to pass extra information, we design that as a new function. I can't see a need for that in a cache-coherent system, and in a non-coherent system the interface needs to be more complicated than just a set of pointers (because you also need to know how big the object at the target of the pointer is). So this gets complicated, but, since we're not going to implement it until we need it and understand the problem better, that’s fine.

I'm avoiding your #2 because I don't believe it is sufficient.

  • Jim

James Cownie <james.h.cownie@intel.com>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)
Tel: +44 117 9071438