From b3aa1385ea6b9c2c8b4a218defe98ec4086bd422 Mon Sep 17 00:00:00 2001 From: Brad Chamberlain Date: Wed, 18 Nov 2020 10:28:54 -0800 Subject: [PATCH 1/4] Convert chpl_nodeID to an `extern proc` Declaring `chpl_nodeID` as a module-scope extern variable pointed to an issue in which we weren't introducing a shadow variable for it as we do for other module-scope variables. However, it also shows a weakness of using a module-scope extern variable for an external value that is different on each locale: it suggests to Chapel that there's a single extern variable when in fact there's a per-locale variable with a distinct value on each locale. This suggests that an extern var isn't the best way to represent such values in Chapel. In this PR, I change chpl_nodeID to an extern (paren-less) proc so that the Chapel code which refers to it can remain unchanged; and I assign it a new name in the C code so that we can implement it as a static inline function without modifying other C code that refers to the C-level variable. The result is a minimal change to our code that avoids the mismatch between describing such a value as a global variable in Chapel that should result in a nearly identical execution-time implementation. --- Signed-off-by: Brad Chamberlain --- modules/internal/LocaleModelHelpSetup.chpl | 2 +- runtime/include/chpl-comm.h | 8 ++++++++ test/localeModels/dmk/locale_name.chpl | 1 - 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/modules/internal/LocaleModelHelpSetup.chpl b/modules/internal/LocaleModelHelpSetup.chpl index a95170b05ee9..24216f79a338 100644 --- a/modules/internal/LocaleModelHelpSetup.chpl +++ b/modules/internal/LocaleModelHelpSetup.chpl @@ -39,7 +39,7 @@ module LocaleModelHelpSetup { config param debugLocaleModel = false; - extern var chpl_nodeID: chpl_nodeID_t; + extern "get_chpl_nodeID" proc chpl_nodeID: chpl_nodeID_t; record chpl_root_locale_accum { var nPUsPhysAcc: atomic int; diff --git a/runtime/include/chpl-comm.h b/runtime/include/chpl-comm.h index ffb5bec0d833..f264cbc1c5b3 100644 --- a/runtime/include/chpl-comm.h +++ b/runtime/include/chpl-comm.h @@ -41,6 +41,14 @@ extern c_nodeid_t chpl_nodeID; // unique ID for each node: 0, 1, 2, ... // the current task is running. // Note also that this value is set only in chpl_comm_init to a value which is // (hopefully) unique to the running image, and never changed again. + +// +// Helper function for Chapel to get the value of chpl_nodeID +// +static inline c_nodeid_t get_chpl_nodeID() { + return chpl_nodeID; +} + extern int32_t chpl_numNodes; // number of nodes size_t chpl_comm_getenvMaxHeapSize(void); diff --git a/test/localeModels/dmk/locale_name.chpl b/test/localeModels/dmk/locale_name.chpl index 058e8ffb239a..6f6e42761dfa 100644 --- a/test/localeModels/dmk/locale_name.chpl +++ b/test/localeModels/dmk/locale_name.chpl @@ -1,5 +1,4 @@ extern proc chpl_nodeName(): c_string; -extern var chpl_nodeID: chpl_nodeID_t; var locale_name = here.chpl_name(); From b52872ff74af0f7137caaad4ea8872065288d7d9 Mon Sep 17 00:00:00 2001 From: Brad Chamberlain Date: Wed, 18 Nov 2020 10:48:06 -0800 Subject: [PATCH 2/4] Add test that wasn't working as expected before, but now does --- Signed-off-by: Brad Chamberlain --- test/localeModels/nodeIDbug.chpl | 23 ++++++++++++++++++++++ test/localeModels/nodeIDbug.comm-none.good | 5 +++++ test/localeModels/nodeIDbug.good | 11 +++++++++++ test/localeModels/nodeIDbug.numlocales | 1 + 4 files changed, 40 insertions(+) create mode 100644 test/localeModels/nodeIDbug.chpl create mode 100644 test/localeModels/nodeIDbug.comm-none.good create mode 100644 test/localeModels/nodeIDbug.good create mode 100644 test/localeModels/nodeIDbug.numlocales diff --git a/test/localeModels/nodeIDbug.chpl b/test/localeModels/nodeIDbug.chpl new file mode 100644 index 000000000000..ef19a220a678 --- /dev/null +++ b/test/localeModels/nodeIDbug.chpl @@ -0,0 +1,23 @@ +var s$: sync int = 0; + +writeln("coforall: "); +coforall loc in Locales { + on loc { + while (s$.readFF() != here.id) { } + s$.readFE(); + writeln("chpl_nodeID: ", chpl_nodeID, ", here.id = ", here.id); + s$.writeEF(here.id+1); + } +} +writeln(); +s$.writeXF(0); + +writeln("forall: "); +forall loc in Locales { + on loc { + while (s$.readFF() != here.id) { } + s$.readFE(); + writeln("chpl_nodeID: ", chpl_nodeID, ", here.id = ", here.id); + s$.writeEF(here.id+1); + } +} diff --git a/test/localeModels/nodeIDbug.comm-none.good b/test/localeModels/nodeIDbug.comm-none.good new file mode 100644 index 000000000000..8ae7967010af --- /dev/null +++ b/test/localeModels/nodeIDbug.comm-none.good @@ -0,0 +1,5 @@ +coforall: +chpl_nodeID: 0, here.id = 0 + +forall: +chpl_nodeID: 0, here.id = 0 diff --git a/test/localeModels/nodeIDbug.good b/test/localeModels/nodeIDbug.good new file mode 100644 index 000000000000..26e74e64cbc5 --- /dev/null +++ b/test/localeModels/nodeIDbug.good @@ -0,0 +1,11 @@ +coforall: +chpl_nodeID: 0, here.id = 0 +chpl_nodeID: 1, here.id = 1 +chpl_nodeID: 2, here.id = 2 +chpl_nodeID: 3, here.id = 3 + +forall: +chpl_nodeID: 0, here.id = 0 +chpl_nodeID: 1, here.id = 1 +chpl_nodeID: 2, here.id = 2 +chpl_nodeID: 3, here.id = 3 diff --git a/test/localeModels/nodeIDbug.numlocales b/test/localeModels/nodeIDbug.numlocales new file mode 100644 index 000000000000..b8626c4cff28 --- /dev/null +++ b/test/localeModels/nodeIDbug.numlocales @@ -0,0 +1 @@ +4 From 8daefc2c832fc3b341175281fa540f9679bd61a7 Mon Sep 17 00:00:00 2001 From: Brad Chamberlain Date: Wed, 18 Nov 2020 12:00:08 -0800 Subject: [PATCH 3/4] Add a missing `void` argument list to get past gcc warnings --- Signed-off-by: Brad Chamberlain --- runtime/include/chpl-comm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/include/chpl-comm.h b/runtime/include/chpl-comm.h index f264cbc1c5b3..c20c45df8e2a 100644 --- a/runtime/include/chpl-comm.h +++ b/runtime/include/chpl-comm.h @@ -45,7 +45,7 @@ extern c_nodeid_t chpl_nodeID; // unique ID for each node: 0, 1, 2, ... // // Helper function for Chapel to get the value of chpl_nodeID // -static inline c_nodeid_t get_chpl_nodeID() { +static inline c_nodeid_t get_chpl_nodeID(void) { return chpl_nodeID; } From d7e8eddfc0abef6b89d2a930d41b0c4343481a0e Mon Sep 17 00:00:00 2001 From: Brad Chamberlain Date: Wed, 18 Nov 2020 14:52:18 -0800 Subject: [PATCH 4/4] Assert that get_chpl_nodeID is synchronization-free --- Signed-off-by: Brad Chamberlain --- modules/internal/LocaleModelHelpSetup.chpl | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/internal/LocaleModelHelpSetup.chpl b/modules/internal/LocaleModelHelpSetup.chpl index 24216f79a338..243ffa7a8c1f 100644 --- a/modules/internal/LocaleModelHelpSetup.chpl +++ b/modules/internal/LocaleModelHelpSetup.chpl @@ -39,6 +39,7 @@ module LocaleModelHelpSetup { config param debugLocaleModel = false; + pragma "fn synchronization free" extern "get_chpl_nodeID" proc chpl_nodeID: chpl_nodeID_t; record chpl_root_locale_accum {