WiP: resolved some minor errors that raised from the initial clone of the branch #45

monoatamd · 2022-11-04T12:02:40Z

Commits 4 & 5: Fixed the issues raised in comments

Added method size() to queueu_record_list_t
Fixed the code to not treat copy like copyin
Removed modified parts which were not in compliance with GPUFORT concept (e.g., gpufortrt_devide_t)

Commit 3: Updated python part

Added a few lines that convert acc library routines to gpufort

Commit 2: additional examples to test acc rtlib

Added some simple examples for testing purposes

Commit 1: resolved minor errors in forked repo

Defined gpufortrt_device_t dev_type = "AMD" by default
Didnt know about the correct value that should be passed to function setup in file queue_record_list_t.cpp. Should it be -1?
Copy and copyin look identical (at the moment). I should further read the specification and implement copy
Commented out (for now) problematic functions like variants of function gpufortrt_get_property.

runtime/gpufortrt/include/gpufortrt_api.h

runtime/gpufortrt/src/gpufortrt_api.cpp

domcharrier · 2022-11-08T08:27:55Z

runtime/gpufortrt/src/gpufortrt_api.cpp

-      HIP_CHECK(hipMemGetInfo(&free, &total))
-      return free;
+    case gpufortrt_property_free_memory:
+      size_t freeMem;


Why the renaming? Usage of camelCase does not fit with rest of code base. I am planning to adopt the LLVM programming style eventually but not for the time being.

In switch case, the scope of variables is the switch not the cases: we get errors about the redefinition of variables free and total.

Either we have to put the definition of free and total in the switch (which looks weird) or we need to define new variables. How about free_mem and total_mem?

fixed in commit 59d4293

Two more things:

Probably makes sense in this case to declare the inout arguments outside
of the case to prevent naming conflicts.

Alternatively, you can use '{' and '}' like this
case <label> { ... } ( don't forget to break )
to have a local scope for variables within the different cases where needed.

Took first option and fixed it in commit a69f450

domcharrier · 2022-11-08T08:31:02Z

python/gpufort/scanner/parser.py

@@ -629,6 +629,7 @@ def is_end_statement_(tokens, kind):
                    current_linemap["statements"]):
                try:
                    expand_statement_functions_(current_statement)
+                    original_statement = current_statement["body"]


Won't accept frontend changes now as I am currently changing it on a separate development branch.
I suggest to keep a local copy and not include to this PR.

Please feel free to reject the changes. I have local copies.

domcharrier · 2022-11-08T08:31:10Z

python/gpufort/util/parsing/parsing.py

@@ -1942,6 +1942,10 @@ def is_declaration(tokens):
 def is_blank_line(statement):
    return not len(statement.strip())

+def is_acc_rtlib(statement, modern_fortran):


Won't accept frontend changes now as I am currently changing it on a separate development branch.
I suggest to keep a local copy and not include to this PR.

Please feel free to reject the changes. I have local copies

domcharrier · 2022-11-08T09:17:42Z

runtime/gpufortrt/src/gpufortrt_api_core.f90

+          !
+      end function
+    end interface
+    if(c_associated(gpufortrt_is_present_c_impl(c_loc(data_arg),int(bytes,kind=c_size_t)))) then


I think you can make an assignment out of this.
Please further do not use the camelCase for the result arg,
breaks with the OpenACC look and feel.
You could also just write

logical :: gpufortrt_is_present ... gpufort_is_present = c_associated(...)

and get rid of the result(...) alltogether.

fixed in commit 59d4293

domcharrier · 2022-11-08T09:20:02Z

runtime/gpufortrt/src/gpufortrt_api_core.f90

@@ -292,4 +292,31 @@ function gpufortrt_use_device_c_impl(hostptr,condition,if_present) &
    resultptr = gpufortrt_use_device_c_impl(hostptr,opt_if_arg,opt_if_present_arg)
  end function

+  function gpufortrt_is_present(data_arg, bytes) result(isOnDeviceMemory) 
+    use iso_c_binding


If not intended (in this case it shouldn't), try not to forget the implicit none after the use statements and before the first declaration / line of code.
Otherwise, Fortran's default implicit naming rules are active, which based on the first letter of an identifier
will assign a default integer or real type to undeclared variables.

fixed in commit 59d4293

domcharrier · 2022-11-08T09:21:15Z

runtime/gpufortrt/src/gpufortrt_api_core.f90

+    interface
+      function gpufortrt_is_present_c_impl(data_arg, bytes) &
+        bind(c,name="gpufortrt_present") result(isOnDeviceMemory) 
+          use iso_c_binding


Also use implicit none here.

Could we use implicit none once at the module scope? This way we don't need to repeat it for every interface/function/subroutine

fixed in commit 59d4293

You could do that (works both for program and module) but it would not apply to the interfaces; see
https://stackoverflow.com/a/24337734
Which can cause confusion.

Ok then, I avoid putting it at module scope to keep it clear

monoatamd · 2022-12-06T10:44:55Z

runtime/gpufortrt/src/gpufortrt_api.f90

+  !> \param[in] if_present Only return device pointer if one could be found for the host pointer.
+  !>                       otherwise host pointer is returned. Defaults to '.false.'.
+  !> \note Returns a c_null_ptr if the host pointer is invalid, i.e. not C associated.
+  subroutine gpufortrt_use_device(resultptr, hostptr,sizes,lbounds,if_arg,if_present) 


I'm working on use_device, as it doesn't work at the moment:

I didnt get the logic in previous implementation.

monoatamd added 2 commits November 4, 2022 12:31

resolved minor errors in forked repo

ca701ff

additional examples to test acc rtlib

1fad76b

monoatamd requested a review from domcharrier November 4, 2022 12:59

domcharrier requested changes Nov 4, 2022

View reviewed changes

domcharrier marked this pull request as draft November 4, 2022 13:10

monoatamd added 4 commits November 7, 2022 08:17

calling gpufortrt instead of acc

08076e0

deleted parts not compliant with gpufort

2929de3

fixed issue with acc_copy and gpufortrt_copy

c040997

Fixed incorrect templates for copy

1706325

domcharrier requested changes Nov 8, 2022

View reviewed changes

acc_is_present impl. with type(*) and dimension

3af85dc

domcharrier requested changes Nov 8, 2022

View reviewed changes

monoatamd added 18 commits November 8, 2022 10:41

avoided camelCase & added implicit none explicitly

59d4293

minor update

af7087e

fixed renaming conflict

a69f450

added list of openacc rtlib routines

aa4c16a

Reverted frontend chngs, added impl. for 4 apis

611c40f

added more high priority APIs

cfd4e14

added more APIs

423d1ac

added acc_get_property

486acd3

added wait and async APIs

081b5ec

making APIs look identical to acc

e92324d

added copyin and copyout

750bda9

Used sizeof instead of size to get num of bytes

0797f5f

added set and get default async

aa63276

added acc_create and test programs

497afdf

added acc_delete and a test program

927a09c

added acc_update/device and a test program

71a809f

added deviceptr and updated implementation_status

9da7378

added acc_get_num_devices and a test program

bec8050

monoatamd added 6 commits November 23, 2022 10:16

added gpufortrt_is_present and added test programs

0562abc

LOG_ERROR updated in gpufortrt_is_present

26587db

added acc_malloc and a test program

84fb3fe

updated acc_malloc & test program, added acc_free

487f985

added acc_map_data and a test program

93abafa

Remved templats & macros. use_device needs revison

3cad2b6

monoatamd commented Dec 6, 2022

View reviewed changes

monoatamd added 2 commits December 8, 2022 15:41

added use_device

246a629

removed optional attrib. from non-optional args.

871c6b5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WiP: resolved some minor errors that raised from the initial clone of the branch #45

WiP: resolved some minor errors that raised from the initial clone of the branch #45

monoatamd commented Nov 4, 2022 •

edited

Loading

domcharrier Nov 8, 2022

monoatamd Nov 8, 2022

monoatamd Nov 8, 2022

domcharrier Nov 8, 2022 •

edited

Loading

monoatamd Nov 8, 2022 •

edited

Loading

domcharrier Nov 8, 2022

monoatamd Nov 8, 2022

domcharrier Nov 8, 2022

monoatamd Nov 8, 2022 •

edited

Loading

domcharrier Nov 8, 2022

monoatamd Nov 8, 2022

domcharrier Nov 8, 2022 •

edited

Loading

monoatamd Nov 8, 2022

domcharrier Nov 8, 2022

monoatamd Nov 8, 2022

monoatamd Nov 8, 2022

domcharrier Nov 8, 2022

monoatamd Nov 8, 2022

monoatamd Dec 6, 2022

WiP: resolved some minor errors that raised from the initial clone of the branch #45

Are you sure you want to change the base?

WiP: resolved some minor errors that raised from the initial clone of the branch #45

Conversation

monoatamd commented Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcharrier Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

monoatamd Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monoatamd Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcharrier Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monoatamd commented Nov 4, 2022 •

edited

Loading

domcharrier Nov 8, 2022 •

edited

Loading

monoatamd Nov 8, 2022 •

edited

Loading

monoatamd Nov 8, 2022 •

edited

Loading

domcharrier Nov 8, 2022 •

edited

Loading