[flang-commits] [PATCH] D150455: [flang][hlfir] Implement the scheduling part of hlfir.forall codegen

Mon May 15 11:51:05 PDT 2023

vzakhari added a comment.

Posting intermediate minor remarks. Still looking through the code...

================
Comment at: flang/include/flang/Optimizer/HLFIR/HLFIROps.td:173
 def hlfir_DesignateOp : hlfir_Op<"designate", [AttrSizedOperandSegments,
-    DeclareOpInterfaceMethods<fir_FortranVariableOpInterface>]> {
+    DeclareOpInterfaceMethods<fir_FortranVariableOpInterface>, NoMemoryEffect]> {
   let summary = "Designate a Fortran variable";
----------------
Is it true for dynamically sized derived types that the `DesignateOp` does not have memory effects?  Does the call generated during `FieldIndexOp` code-gen access any memory?

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp:38
+    "flang-dbg-order-assignment-schedule-only",
+    llvm::cl::desc("Lower allocations to fortran runtime calls"),
+    llvm::cl::init(false));
----------------
The description string looks off.

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp:42
+/// Shared rewrite entry point for all the ordered assignment tree root
+/// operations. It calls he scheduler and then apply the schedule.
+static mlir::LogicalResult
----------------
typo

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp:50
+      /// Debug option to print the scheduling debug info without doing
+      /// any code generation. The operation are simply erased to avoid
+      /// failing and calling the rewrite patterns on nested operations.
----------------
typo

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:1
+//===- ScheduleOrderedAssignments.cpp -- Schedule HLFIR ordered assignments
+//-===//
----------------
nit: maybe shrink the comment to fit the line limit.

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:50
+/// Structure that is in charge of building the schedule. For each
+/// hlfir.region_assign inside and ordered assignment tree, it is walked through
+/// the parent operations and their "leaf" regions (that contain expression
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:66
+
+  /// Start analysing a set of evaluation regions that are can be evaluated in
+  /// any order between themselves according to Fortran rules (like the controls
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:70
+  /// independent evaluations to a run that would save only one of the control.
+  void startIndependentEvaluationGroup(){};
+
----------------
It is not doing anything, is this WIP?

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:77
+  /// evaluating the current assignment. For expression with write effect,
+  /// saving them ensures they are evaluated only once. An region whose value
+  /// was saved in a previous run is considered to have no side effects with the
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:98
+
+  /// Once all the assignments have been analyzed and schedule, return the
+  /// schedule. The scheduler object should not be used after this call.
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:104
+  /// Save a conflicting region that is evaluating an expression that is
+  /// controlling or masking the current assignment, or is evaluating is
+  /// RHS/LHS.
----------------
typo: `is evaluating is`

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:111
+
+  /// Can the current assignment be schedule with the previous run. This / is
+  /// only possible if the assignment and all of its dependencies have no side
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.cpp:199
+    // they are allocatable/pointer allocation/deallocation, they are conveyed
+    // via the write that is updating the descriptor/allocatable (and their
+    // cannot be any indirect allocatable/pointer allocation/deallocation if
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:1
+//===- ScheduleOrderedAssignments.h -- Schedule HLFIR ordered assignments -===//
+//
----------------
nit: `-*- C++ -*-` missing.

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:1
+//===- ScheduleOrderedAssignments.h -- Schedule HLFIR ordered assignments -===//
+//
----------------
vzakhari wrote:
> nit: `-*- C++ -*-` missing.

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:14
+
+#ifndef OPTIMIZER_HLFIR_TRANSFORM_SCHEDULEORDEREDASSIGNMENTS_H
+#define OPTIMIZER_HLFIR_TRANSFORM_SCHEDULEORDEREDASSIGNMENTS_H
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:15
+#ifndef OPTIMIZER_HLFIR_TRANSFORM_SCHEDULEORDEREDASSIGNMENTS_H
+#define OPTIMIZER_HLFIR_TRANSFORM_SCHEDULEORDEREDASSIGNMENTS_H
+
----------------

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:32
+
+/// A run is a list of actions required to evaluate and ordered assignment tree
+/// that can be done in the same loop nest.
----------------
typo

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:68
+///  will generate 2 runs for the schdule. The first containing
+///  SavedEntity{rhs_region}, and the second one containing the
+///  hlfir.region_assign.
----------------
typo

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:76
+///   inside,
+///      the loop, but do not evaluate the LHS or assignment.
+///   For the second run:
----------------
nit: line alignment

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:84
+/// generate only one run with the hlfir.region_assign, which will be
+/// implemented as a single loops that evaluate %xi, %yi and does %xi = %yi in
+/// the loop body.
----------------
typo

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:90
+/// The main goal of the scheduler is to avoid creating temporary storage
+/// (required for SavedEntity). But it can optionally be asked to fuse Forall
+/// and Where assignments in the same loop nests when possible since it has the
----------------
typo

================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:97
+} // namespace hlfir
+#endif /*SCHEDULEORDEREDASSIGNMENTS*/
----------------
nit

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150455/new/

https://reviews.llvm.org/D150455