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

[Jean Perier via Phabricator via flang-commits]

jeanPerier marked 28 inline comments as done.
jeanPerier added a comment.

Thanks a lot for the review and comments!



================
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";
----------------
vzakhari wrote:
> 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?
I do not have a good answer here because it is still not 100% clear to me how this will be dealt with. The runtime approach lays out dynamically sized component in "automatic components" that are special allocatable components.

So if this is not abstracted in FIR/HLFIR, the memory effects will happen via a fir.load of the addresses component fir.box (like for allocatable components).

The FieldIndexOp codegen was forcasting an approach where the data would be directly laid out in the structure with no descriptor indirection.

Right now, it is safe to say it has no memory effects (it may however crash if the incoming fir.box is an absent optional, that is why I did not add Pure: it is not speculatable with optional input for now).

We will likely need to revisit this depending on how PDTs are implemented (we can implement the MemoryEffectInterface to have finer control about when an operation has side effects and when it does not based on the arguments).


================
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));
----------------
vzakhari wrote:
> The description string looks off.
Thanks


================
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(){};
+
----------------
vzakhari wrote:
> It is not doing anything, is this WIP?
There is no much to do here, I added it because there is something to do after visiting the group, and I found it weird to not mark the beginning.

I added an assert to make it useful.


================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderAssignments.h:1
+//===- ScheduleOrderedAssignments.h -- Schedule HLFIR ordered assignments -===//
+//
----------------
vzakhari wrote:
> vzakhari wrote:
> > nit: `-*- C++ -*-` missing.
> 
Actually, I need to rename the files instead to be consistent. Thanks for the catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150455