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

John McCall rjmccall at gmail.com
Tue Nov 18 14:24:10 PST 2014


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






More information about the cfe-commits mailing list