archspec / archspec-json

Other
20 stars 32 forks source link

add nvhpc support #67

Closed cparrott73 closed 1 year ago

cparrott73 commented 1 year ago

Rebased from archspec-json master as of 06-Apr-2023.

Please let me know if I need to fix something here. Thanks.

alalazo commented 1 year ago

@glennpj Since you were in the loop in the initial PR #52, I was wondering if you are still interested in having a look at this one :slightly_smiling_face:

glennpj commented 1 year ago

Sure, I can take a look.

glennpj commented 1 year ago

@alalazo I am trying to test this in Spack but the flags are not getting pulled in. Is there something more to do other than dropping the json file in place?

spack python -c "import archspec; archspec.cpu.TARGETS['icelake'].optimization_flags('nvhpc', '23')"
''
alalazo commented 1 year ago

@glennpj I can reproduce. Checking this right now

alalazo commented 1 year ago

@glennpj So, this is due to missing entries for x86_64 and x86_64_v2. When designing the Microarchitecture class, one assumption was that, if a compiler can emit code for a specific microarchitecture, it can also emit code for a more generic target. This seems not to be the case for nvhpc? @cparrott73

cparrott73 commented 1 year ago

@alalazo That is correct. I would have to go dig up the exact version where this went into effect, but for the past couple of years or so, the nvhpc compilers require CPUs that support AVX instructions at minimum (Sandy Bridge or later). They no longer have the ability to generate generic x86_64 instructions.

alalazo commented 1 year ago

@glennpj This diff on archspec should be able to get you going:

diff --git a/archspec/cpu/microarchitecture.py b/archspec/cpu/microarchitecture.py
index 471c6f2..7912035 100644
--- a/archspec/cpu/microarchitecture.py
+++ b/archspec/cpu/microarchitecture.py
@@ -209,7 +209,7 @@ class Microarchitecture:
             version (str): version of the compiler to be used
         """
         # If we don't have information on compiler at all return an empty string
-        if compiler not in self.family.compilers:
+        if compiler not in self.generic.compilers:
             return ""

         # If we have information but it stops before this

while I think of a more stable solution to apply in the archspec.

glennpj commented 1 year ago

The patch to microarchitecture.py does get things working for targets that have a functioning entry. However, the following micro-architecture entries:

have entries for nvhpc but the flags are not passed because there is no entry for nvhpc from the inherited micro-architecture, ie., generic. Perhaps this is okay for the archspec-json git repo and a fix needs to go in the archspec git repo to make those targets usable. Another thought is to have generic entries and turn features off from the default.

alalazo commented 1 year ago

Perhaps this is okay for the archspec-json git repo and a fix needs to go in the archspec git repo to make those targets usable. Another thought is to have generic entries and turn features off from the default.

Yeah, I need to figure out what would be a good solution for this. I'll try to code something this week, and if you agree I'll ping you in the archspec/archspec PR I'll submit.

alalazo commented 1 year ago

@cparrott73 Can you add:

        "nvhpc" : []

entries in the targets that are more generic than ones already handled, but are not supported by nvhpc? Or I can do it if you don't mind me pushing to your PR.

cparrott73 commented 1 year ago

@alalazo Sure. I wasn't sure what the proper semantics would be to handle these sorts of cases. I can add these entries.

We have similar considerations for POWER and ARM as well - I think anything lower than POWER 8 (little-endian) and ARM v8.1 are not support by our compilers, either.

alalazo commented 1 year ago

I wasn't sure what the proper semantics would be to handle these sorts of cases.

nvhpc is the first case of a compiler not supporting generic targets, so there was no proper semantic before :grimacing:

I checked the code on the Python side and I think the easiest and most consistent way to deal with this is to use an empty list to indicate that we know a target is not supported by a compiler. Absence of the entry means instead we don't have information about that compiler.

alalazo commented 1 year ago

@glennpj You should be able to test this one with the empty lists added. If the PR looks good to you I'd merge it.

glennpj commented 1 year ago

if you agree I'll ping you in the archspec/archspec PR I'll submit.

Yes, I agree, please ping me.