[flang] Fix invalid diagnostics in wrapper and reduce code dup.

[awarzynski]

Hey @ygribov, thanks for submitting this!

Fix invalid diagnostics

Which diagnostics are invalid?

flang/tools/f18/flang

282

It would be great if this name could follow's LLVM's coding guide:

Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

Perhaps run_and_check_status? run_compiler_command? run_and_check? run_cmd?

Also, could you add a short description?

321

Similar suggestion elsewhere

[ygribov]

In D118618#3287786, @awarzynski wrote:

Hey @ygribov, thanks for submitting this!

Fix invalid diagnostics

Which diagnostics are invalid?

The error message included invalid command-line parameters for flang's invocation.

[ygribov]

flang/tools/f18/flang

282

run_and_check sounds fine and indeed I forgot description.

[awarzynski]

This change makes a lot of sense and is a big improvement.

I just feel that the summary focus on one of the side effects rather than what this patch really does. I think that the main thing here is the refactoring and the code-duplication reduction. The inconsistent diagnostic is also fixed, but that's a relatively small change (hence "side-effect") and could be achieved without the refactoring. So, I would update the summary :) This is not a blocker for me though!

flang/tools/f18/flang

319

This seems to be the only place in which the diagnostic is inconsistent with the compiler invocation (should be $ect_fc instead of "$wd/bin/flang-new").

[kiranchandramohan]

Thanks @ygribov for this patch and the other contributions made.
We will take this patch. I just wanted to highlight that the flang script is temporary and will be replaced by the flang-new driver when codegen is upstreamed. I guess the longer-standing contributions will be in flang-new. We are also trying to minimize changes to the flang script to keep it stable. Sometimes we have seen minor changes running into issues in a different bash version. Hope that is OK with you.

[ygribov]

In D118618#3289849, @kiranchandramohan wrote:

Thanks @ygribov for this patch and the other contributions made.
We will take this patch. I just wanted to highlight that the flang script is temporary and will be replaced by the flang-new driver when codegen is upstreamed. I guess the longer-standing contributions will be in flang-new. We are also trying to minimize changes to the flang script to keep it stable. Sometimes we have seen minor changes running into issues in a different bash version. Hope that is OK with you.

Hi Kiran,

Thanks for the update. We are just trying to figure out the status of flang's frontend at the moment and fix issues in flang script which we run into. I do realize that this work will be thrown away in few months. If you believe that this is not really useful, I'll keep flang fixes in our local branch.

As a side note, what is the plan about merging codegen into main?

[ygribov]

flang/tools/f18/flang

319

Not quite, I believe command-line arguments are wrong in practically all invocations...

[awarzynski]

In D118618#3289948, @ygribov wrote:

We are just trying to figure out the status of flang's frontend at the moment

Why not use flang-new for this? The main difference between flang-new and flang is that the latter calls an external Fortran compiler to compile the unparsed source files. If you do need code-generation, then that's available on fir-dev. We've been using it to compile e.g. SNAP (i.e. the driver is already fairly capable).

If you believe that this is not really useful, I'll keep flang fixes in our local branch.

All improvements are useful. But we have no upstream testing for this script and a few downstream users to rely on this. Personally, I'd love for everyone to switch to flang-new instead. And if there's anything that's missing in flang-new that prevents people from switching, it would be good to identify such blockers.

As a side note, what is the plan about merging codegen into main?

It's work-in-progress. We'd like it to happen ASAP. As for the driver itself, I want to start moving code-gen patches from fir-dev onto main in the coming days. I just keep getting distracted :-)

flang/tools/f18/flang

319

Could you be more specific? The compiler invocation in this case is:

$ext_fc -E "${opts[@]}" "${other_srcs[$idx]}" ${output_definition:+$output_definition}

and the diagnostic is:

flang-new failed with exit status $status: "$wd/bin/flang-new" "${opts[@]}" "$@"

So, flang-new should be replaced with $ext_fc. Also, "$@" should be removed. Anything else? What about other places?

I'm just trying to understand the behavior "before" and "after" this change. Documenting this would be much appreciated (either as a comment here or in the summary). It would really help anyone browsing through the history of this script (in case something breaks, which usually happens to downstream users).

[ygribov]

Closing.

flang/tools/f18/flang

get_external_fc_name() {
   elif [[ -v F18_FC ]]; then
     # We support F18_FC for backwards compatibility.
     echo ${F18_FC}
   else
     echo gfortran
   fi
 }
 
+run_checked() {
+  if ! "$@"
+  then status=$?
+       echo flang: in "$PWD", $1 failed with exit status $status: "$@" >&2
+       exit $status
+  fi
+}
+
 # === preprocess ==============================================================
 #
 # Runs the preprocessing. Fortran files are preprocessed using Flang. Other
 # files are preprocessed using the external Fortran compiler.
 #
 # INPUTS:
 #   $1 - Fortran source files (array, name reference)
 #   $2 - other source files (array, name reference)
preprocess() {
 
   # Use the provided output file name.
   if [[ ! -z ${OUTPUT_FILE:+x} ]]; then
     output_definition="-o $OUTPUT_FILE"
   fi
 
   # Preprocess fortran sources using Flang
   for idx in "${!fortran_srcs[@]}"; do
-    if ! "$wd/bin/flang-new" -E "${opts[@]}" "${fortran_srcs[$idx]}" ${output_definition:+$output_definition}
-    then status=$?
-         echo flang: in "$PWD", flang-new failed with exit status $status: "$wd/bin/flang-new" "${opts[@]}" "$@" >&2
-         exit $status
-    fi
+    run_checked "$wd/bin/flang-new" -E "${opts[@]}" "${fortran_srcs[$idx]}" ${output_definition:+$output_definition}
   done
 
   # Preprocess other sources using Flang
   for idx in "${!other_srcs[@]}"; do
-    if ! $ext_fc -E "${opts[@]}" "${other_srcs[$idx]}" ${output_definition:+$output_definition}
-    then status=$?
-         echo flang: in "$PWD", flang-new failed with exit status $status: "$wd/bin/flang-new" "${opts[@]}" "$@" >&2
-         exit $status
-    fi
+    run_checked $ext_fc -E "${opts[@]}" "${other_srcs[$idx]}" ${output_definition:+$output_definition}
   done
 }
 
 # === get_relocatable_name ======================================================
 # This method generates the name of the output file for the compilation phase
 # (triggered with `-c`). If the user of this script is only interested in
 # compilation (`flang -c`), use $OUTPUT_FILE provided that it was defined.
 # Otherwise, use the usual heuristics:
main() {
   flang_options+=("-module-suffix")
   flang_options+=(".f18.mod")
   flang_options+=("-fdebug-unparse")
   flang_options+=("-fno-analyzed-objects-for-unparse")
 
   [[ ! -z ${MODULE_DIR} ]] && flang_options+=("-module-dir ${MODULE_DIR}")
   [[ ! -z ${INTRINSICS_MOD_DIR} ]] && flang_options+=("-intrinsics-module-directory ${INTRINSICS_MOD_DIR}")
   for idx in "${!fortran_source_files[@]}"; do
-    set +e
-    "$wd/bin/flang-new" "${flang_options[@]}" "${fortran_source_files[$idx]}" -o "${unparsed_file_base}_${idx}.f90"
-    ret_status=$?
-    set -e
-    if [[ $ret_status != 0 ]]; then
-         echo flang: in "$PWD", flang-new failed with exit status "$ret_status": "$wd/bin/flang-new" "${flang_options[@]}" "$@" >&2
-         exit "$ret_status"
-    fi
+    run_checked "$wd/bin/flang-new" "${flang_options[@]}" "${fortran_source_files[$idx]}" -o "${unparsed_file_base}_${idx}.f90"
   done
 
   # STEP 2: Compile Fortran Source Files
   local ext_fc="$(get_external_fc_name)"
   # Temporary object files generated by this script. To be deleted at the end.
   local temp_object_files=()
   for idx in "${!fortran_source_files[@]}"; do
     # We always have to specify the output name with `-o <out_obj_file>`. This
     # is because we are using the unparsed rather than the original source file
     # below. As a result, we cannot rely on the compiler-generated output name.
     out_obj_file=$(get_relocatable_name "${fortran_source_files[$idx]}")
 
-    set +e
-    $ext_fc "-c" "${ext_fc_options[@]}" "${unparsed_file_base}_${idx}.f90" "-o" "${out_obj_file}"
-    ret_status=$?
-    set -e
-    if [[ $ret_status != 0 ]]; then
-      echo flang: in "$PWD", "$ext_fc" failed with exit status "$ret_status": "$ext_fc" "${ext_fc_options[@]}" "$@" >&2
-         exit "$ret_status"
-    fi
+    run_checked $ext_fc "-c" "${ext_fc_options[@]}" "${unparsed_file_base}_${idx}.f90" "-o" "${out_obj_file}"
     temp_object_files+=(${out_obj_file})
   done
 
   # Delete the unparsed files
   for idx in "${!fortran_source_files[@]}"; do
     rm "${unparsed_file_base}_${idx}.f90"
   done
 
   # STEP 3: Compile Other Source Files
   for idx in "${!other_source_files[@]}"; do
     # We always specify the output name with `-o <out_obj_file>`. The user
     # might have used `-o`, but we never add it to $OPTIONS (or
     # $ext_fc_options). Hence we need to use `get_relocatable_name`.
     out_obj_file=$(get_relocatable_name "${other_source_files[$idx]}")
 
-    set +e
-    $ext_fc "-c" "${ext_fc_options[@]}" "${other_source_files[${idx}]}" "-o" "${out_obj_file}"
-    ret_status=$?
-    set -e
-    if [[ $ret_status != 0 ]]; then
-      echo flang: in "$PWD", "$ext_fc" failed with exit status "$ret_status": "$ext_fc" "${ext_fc_options[@]}" "$@" >&2
-         exit "$ret_status"
-    fi
+    run_checked $ext_fc "-c" "${ext_fc_options[@]}" "${other_source_files[${idx}]}" "-o" "${out_obj_file}"
     temp_object_files+=(${out_obj_file})
   done
 
   # STEP 4: Link
   if [[ $COMPILE_ONLY == "True" ]]; then
     exit 0;
   fi
 
   if [[ ${#temp_object_files[@]} -ge 1 ]] || [[ ${#object_files[@]} -ge 1 ]] ; then
     # If $OUTPUT_FILE was specified, use it for the output name.
     if [[ ! -z ${OUTPUT_FILE:+x} ]]; then
       output_definition="-o $OUTPUT_FILE"
     else
       output_definition=""
     fi
 
-    set +e
-    $ext_fc "${ext_fc_options[@]}" "${object_files[@]}" "${temp_object_files[@]}" "${lib_files[@]}" ${output_definition:+$output_definition}
-    ret_status=$?
-    set -e
-    if [[ $ret_status != 0 ]]; then
-         echo flang: in "$PWD", "$ext_fc" failed with exit status "$ret_status": "$ext_fc" "${ext_fc_options[@]}" "$@" >&2
-         exit "$ret_status"
-    fi
+    run_checked $ext_fc "${ext_fc_options[@]}" "${object_files[@]}" "${temp_object_files[@]}" "${lib_files[@]}" ${output_definition:+$output_definition}
   fi
 
   # Delete intermediate object files
   for idx in "${!fortran_source_files[@]}"; do
     rm "${temp_object_files[$idx]}"
   done
 }
 
 main "${@}"