-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Flang (Classic) compiler #1538
Conversation
@@ -67,7 +67,7 @@ MODULE NWTC_Base | |||
|
|||
INTEGER(C_INTPTR_T) :: FileAddr = INT(0,C_INTPTR_T) !< The address of file FileName. (RETURN value from LoadLibrary ) [Windows] | |||
TYPE(C_PTR) :: FileAddrX = C_NULL_PTR !< The address of file FileName. (RETURN value from dlopen ) [Linux] | |||
TYPE(C_FUNPTR) :: ProcAddr(NWTC_MAX_DLL_PROC) = C_NULL_FUNPTR !< The address of procedure ProcName. (RETURN value from GetProcAddress or dlsym) [initialized to Null for pack/unpack] | |||
TYPE(C_FUNPTR) :: ProcAddr(NWTC_MAX_DLL_PROC) !< The address of procedure ProcName. (RETURN value from GetProcAddress or dlsym) [initialized to Null for pack/unpack] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this ProcAddr
isn't initialized, does it cause issues with restart capabilities? I wonder if the uninitialized value becomes a problem when we pack/unpack the array like the comment (from a LONG time ago) mentions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that that it would be better to have it initialized. I'll see if it will accept initialization to another value. Unfortunately this compiler thinks that C_NULL_FUNPTR
isn't valid in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can be wrapped in a preprocessor directive to be safe? Otherwise, we might need to look at all the time ProcAddr is used and nullify it after the object is created.
I recall adding a safety in the DLLTypeUnPack
to enforce that these variables are nullified after unpacking if no DLL is used. 39d37e6
But I don't know if such init are needed elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a way to fix this in the latest commit by creating an array of null function pointers.
@@ -67,7 +67,7 @@ MODULE NWTC_Base | |||
|
|||
INTEGER(C_INTPTR_T) :: FileAddr = INT(0,C_INTPTR_T) !< The address of file FileName. (RETURN value from LoadLibrary ) [Windows] | |||
TYPE(C_PTR) :: FileAddrX = C_NULL_PTR !< The address of file FileName. (RETURN value from dlopen ) [Linux] | |||
TYPE(C_FUNPTR) :: ProcAddr(NWTC_MAX_DLL_PROC) = C_NULL_FUNPTR !< The address of procedure ProcName. (RETURN value from GetProcAddress or dlsym) [initialized to Null for pack/unpack] | |||
TYPE(C_FUNPTR) :: ProcAddr(NWTC_MAX_DLL_PROC) !< The address of procedure ProcName. (RETURN value from GetProcAddress or dlsym) [initialized to Null for pack/unpack] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can be wrapped in a preprocessor directive to be safe? Otherwise, we might need to look at all the time ProcAddr is used and nullify it after the object is created.
I recall adding a safety in the DLLTypeUnPack
to enforce that these variables are nullified after unpacking if no DLL is used. 39d37e6
But I don't know if such init are needed elsewhere.
- A method was found to initial ProcAddr in DLL_Type by creating an array of null pointers (seems like a hack) - Comments were added in SD_FEM.f90 and SubDyn.f90 explaining that changes were made to satisfy the Flang compiler
@@ -58,16 +58,17 @@ MODULE NWTC_Base | |||
|
|||
INTEGER(IntKi), PARAMETER :: NWTC_MAX_DLL_PROC = 5 !< maximum number of procedures that can be dynamically loaded from a DLL (see DLL_Type nwtc_base::dll_type) | |||
|
|||
TYPE(C_FUNPTR), PARAMETER :: NULL_PROC_ADDR(NWTC_MAX_DLL_PROC) = C_NULL_FUNPTR !< this is a hack so the Flang compiler will initialize ProcAddr to C_NULL_FUNPTR in DLL_Type (remove if no longer needed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a array of null function pointers seemed to be the only way for Flang to allow initialization of ProcAddr
in DLL_Type
. This will provide the same initialization as before, it just required an additional line of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's elegant!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging, I discovered this line causes a seg-fault with gfortran 10.4.0
Follow up on this PR. I am getting a segmentation fault in the compiler (gfortran 10.4) when compiling This was working prior to this PR merge:
|
PR OpenFAST#1538 introduced a seg-fault with some gfortran compilers. Added an #ifdef for the Flang compiler
This pull request is ready to be merged pending regression and unit test results.
Feature or improvement description
This PR adds support for the Classic Flang compiler (https://github.com/flang-compiler/flang) used by AMD ROCm (https://www.amd.com/en/graphics/servers-solutions-rocm). This compiler suite is used on the Crusher and Frontier super computers, possibly others.
Impacted areas of the software
This PR mainly affects the build system and NWTC-Library; however, slight changes were required in SubDyn, AeroDyn14, and BeamDyn as the compiler is not fully standards compliant.
SubDyn required removal of
move_alloc
in SD_FEM.f90, though the specific reason is unknown since this subroutine is used elsewhere without problems. For this specific case, the array sizes after being moved were incorrect which caused the subsequent code to fail. Also, when reading the SubDyn input file, carriage returns had to be explicitly removed before callingReadIAryFromStrSD
; otherwise, the function failed to parse the integers correctly.AeroDyn14 input file parsing was changed because the read function considers reading the end of the file to be an error instead of an end. Building the
dwm_driver_wind_farm
was also disabled as it relies on therename
intrinsic which is nonstandard Fortran.BeamDyn failed when reading the blade input file as the whitespace around the mass and stiffness matrices were considered significant. As a result, code was added to read these lines as comments. This will make the parser fail if these lines do not exist in the input files.
Additional supporting information
Additional background on the Classic Flang compiler can be found at https://github.com/flang-compiler/flang/wiki. This is not the same Flang compiler as LLVM Flang (https://flang.llvm.org/docs/) though support will likely need to be added for it in the future.
Test results, if applicable
13 out of 90 regression tests fail on this compiler due to a combination of producing different results and runtime failures. The runtime failures are due to an issue in the random number generator included by the compiler and must be resolved by Flang or AMD (flang-compiler/flang#691 has not been included in AMD's fork of Flang).
In addition, the pFunit test framework does not support Flang so unit testing was not available. I'd like to look into replacing pFunit with https://github.com/fortran-lang/test-drive in the future to resolve this issue.