[PATCH] D111288: [fir] Add data flow optimization pass

Sun Oct 17 17:27:11 PDT 2021

mehdi_amini added inline comments.

================
Comment at: flang/test/Fir/memref-data-flow.fir:103
+// CHECK:    return
+// CHECK:  }
+
----------------
clementval wrote:
> mehdi_amini wrote:
> > clementval wrote:
> > > kiranchandramohan wrote:
> > > > clementval wrote:
> > > > > clementval wrote:
> > > > > > awarzynski wrote:
> > > > > > > clementval wrote:
> > > > > > > > mehdi_amini wrote:
> > > > > > > > > clementval wrote:
> > > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > > These tests looks like entirely auto-generated, what should we expect here?
> > > > > > > > > > > 
> > > > > > > > > > > Also the source of the test seems like taken directly from a real-world example, without any attempt to document or minimize: this can be a maintenance problem moving forward.
> > > > > > > > > > I heard that when you guys discussed the upstreaming of fir-dev it was advise to use to MLIR script to generate the check because the current checks were too small. 
> > > > > > > > > I'm not aware of this, we have documented guidelines though: https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices
> > > > > > > > Thanks for the guidelines link. I'll have to check what we will do in Flang because I now have contradictory information from two sides. 
> > > > > > > > I heard that when you guys discussed the upstreaming of fir-dev it was advise to use to MLIR script to generate the check because the current checks were too small.
> > > > > > > 
> > > > > > > I think that quite a few separate discussions have happened in the last year or so. I was just a by-stander most of the time, so can't really comment. Also, I'm not aware of anything public that we could refer to here (e.g. an RFC or something similar). Perhaps this particular aspect has not been discussed? :)
> > > > > > > 
> > > > > > > @mehdi_amini, 
> > > > > > > > These tests looks like entirely auto-generated, what should we expect here?
> > > > > > > IIUC, you *are* in favor of auto-generated tests (e.g. with [[ https://github.com/llvm/llvm-project/blob/main/mlir/utils/generate-test-checks.py | generate-test-checks.py ]]), but would prefer them to be a bit refactored, right? Otherwise it's not really obvious "what" is being tested. Basically,
> > > > > > > >     Tests should be minimal, and only check what is absolutely necessary.
> > > > > > > 
> > > > > > > @clementval The check lines could be reduced to only contain the key operations being tested and making sure the these are generated correctly.
> > > > > > > 
> > > > > > > > this can be a maintenance problem moving forward
> > > > > > > 
> > > > > > > I agree. Any change in MLIR that causes these tests to fail could be non-trivial to triage. But I also think that it will be quite tricky to re-generate these tests from scratch. Perhaps we could make sure that:
> > > > > > > * every test is documented (e.g. by making a descriptive function name that highlights "what" is being tested)
> > > > > > > * check lines not strictly related to the functionality being tested are reduced/removed
> > > > > > > This way we would make any potential triaging much easier for ourselves without having to re-write these cases.
> > > > > > > 
> > > > > > > This is just my 2p. WDYT?
> > > > > > I fully understand your points @awarzynski and it makes sense to me. Once again I just want to point out that we need to find a trade off in the current upstreaming process. 
> > > > > > I'll not be able to refactor/rework deeply every single patch I post. Keep in mind that every change done here must be sync back in fir-dev until we can discard it (tests are the easy one to sync). `fir-dev` is still evolving everyday and if we want to get rid of it one day we need to find the right pace to the upstreaming can catch up with the daily evolution. 
> > > > > > This is not a comment specifically for this patch but rather a general one for the current process we are in. AFAIK, I'm the only person spending time upstreaming `fir-dev` at the moment. 
> > > > > > In most cases I'm not the author of the code and even refactoring tests can take quite some times. Time that I'm happy to spend when we find flaw or blockers.
> > > > > > We agreed in the past that we will try our best to make small patches because a single push like it was done for MLIR or the Flang frontends was pushed back. 
> > > > > > We might need a new discussion on the usptreaming process if we want it to be successful in a timely manner. 
> > > > > > 
> > > > > A simplification of the first test could look like the following. Would that be ok? 
> > > > > 
> > > > > ```
> > > > > // CHECK-LABEL:   func @_QPf1dc(
> > > > > // CHECK-SAME:                  %[[VAL_0:.*]]: !fir.ref<!fir.array<60xi32>>, 
> > > > > // CHECK:         ^bb2:
> > > > > // CHECK:           %[[VAL_12:.*]] = fir.convert %{{.*}} : (index) -> i32
> > > > > // CHECK-NOT:       fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
> > > > > // CHECK-NOT:       %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
> > > > > // CHECK:           %[[VAL_13:.*]] = fir.convert %[[VAL_12]] : (i32) -> i64
> > > > > 
> > > > > // CHECK:    %[[VAL_15:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
> > > > > // CHECK:    %[[VAL_16:.*]] = fir.load %[[VAL_15]] : !fir.ref<i32>
> > > > > 
> > > > > // CHECK:    %[[VAL_18:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
> > > > > // CHECK:    fir.store %{{.*}} to %[[VAL_18]] : !fir.ref<i32>
> > > > > 
> > > > > // CHECK:  ^bb3:
> > > > > // CHECK:     %[[VAL_21:.*]] = fir.convert %{{.*}} : (index) -> i32
> > > > > // CHECK-NOT: fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
> > > > > // CHECK:     br ^bb4
> > > > > 
> > > > > // CHECK:  ^bb5:
> > > > > // CHECK:    %[[VAL_25:.*]] = fir.convert %{{.*}} : (index) -> i32
> > > > > // CHECK-NOT: fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
> > > > > // CHECK-NOT: %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
> > > > > // CHECK:    %[[VAL_26:.*]] = fir.convert %[[VAL_25]] : (i32) -> i64
> > > > > // CHECK:    %[[VAL_28:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
> > > > > // CHECK:    %[[VAL_29:.*]] = fir.load %[[VAL_28]] : !fir.ref<i32>
> > > > > // CHECK:    %[[VAL_30:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
> > > > > // CHECK:    %[[VAL_31:.*]] = fir.load %[[VAL_30]] : !fir.ref<i32>
> > > > > // CHECK:    %[[VAL_33:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.array<60xi32>>, i64) -> !fir.ref<i32>
> > > > > // CHECK:    fir.store %{{.*}} to %[[VAL_33]] : !fir.ref<i32>
> > > > > ```
> > > > Many thanks, @clementval for your efforts in restarting and making quite some progress with upstreaming. I am in favour of upstreaming fast so that we do not have to work across two repositories (llvm-project and fir-dev). For the OpenMP work, we face some difficulties working across the two repos. 
> > > > 
> > > > I think we can consider,
> > > > 1) Increasing the number of people working on upstreaming. It might be possible (subject to approval) for one of us at Arm to join the upstreaming effort (either in helping to write or fix tests or regular upstreaming work). Will this be acceptable?
> > > > 2) Adding non-critical but essential changes as bugs/issues and come back to this once we have llvm-project and fir-dev on par.
> > > > 3) Stopping fir-dev work once we reach full Fortran 95 support and everyone joins upstreaming for a couple of months.
> > > > I think we can consider,
> > > > 1) Increasing the number of people working on upstreaming. It might be possible (subject to approval) for one of us at Arm to join the upstreaming effort (either in helping to write or fix tests or regular upstreaming work). Will this be acceptable?
> > > Of course more people working on this will help. We just need to coordinate if this is going to happen.
> > > > 2) Adding non-critical but essential changes as bugs/issues and come back to this once we have llvm-project and fir-dev on par.
> > > I think we should go this route. I would suggests that critical changes should be marked as "Request Changes" and any other comment would be done as best effort. Would this be acceptable? Also it doesn't mean that this comments will never be addressed but rather delayed until we can discard fir-dev. 
> > > > 3) Stopping fir-dev work once we reach full Fortran 95 support and everyone joins upstreaming for a couple of months.
> > > I think we should at least reduce what we accept on fir-dev once F95 is reached. I guess we can talk about this during the next call. 
> > > 
> > > 
> > I think we can defer some action items, but I would prefer that a bug is filed and mentioned in a comment like this strawman: `// TODO: llvm.org/pr12345 this needs to be ....`
> > 
> > We can either keep track of these with a "tag"/"keyword" on the bug, or with a separate "component" on buganizer.
> Ok we will coordinate on this. 
> 
> @mehdi_amini Is the smaller test proposed by me or @awarzynski look ok to you? If so, I can update this patch so we can move forward, 
I am not sure: I haven't spent time on the existing test, but I'd start with the function name which isn't descriptive: can we name the function against what it is testing?
In some cases we may need comments about what is the test exercising as well.

I see tests very much as part of the documentation for the code: in many case when I'm unsure about some part of a pass, I'll try to disable it in the code and run the tests to see which case is breaking for example.  But having over-constrained test or test where the input IR isn't minimized makes it impossible.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111288