Page MenuHomePhabricator

[fir] Add array value copy pass
AcceptedPublic

Authored by clementval on Oct 7 2021, 12:06 PM.

Details

Summary

This patch upstream the array value copy pass.

Transform the set of array value primitives to a memory-based array
representation.

The Ops array_load, array_store, array_fetch, and array_update are
used to manage abstract aggregate array values. A simple analysis is done
to determine if there are potential dependences between these operations.
If not, these array operations can be lowered to work directly on the memory
representation. If there is a potential conflict, a temporary is created
along with appropriate copy-in/copy-out operations. Here, a more refined
analysis might be deployed, such as using the affine framework.

This pass is required before code gen to the LLVM IR dialect.

This patch is part of the upstreaming effort from fir-dev branch. The
pass is bringing quite a lot of file with it.

Co-authored-by: Jean Perier <jperier@nvidia.com>
Co-authored-by: Eric Schweitz <eschweitz@nvidia.com>
Co-authored-by: V Donaldson <vdonaldson@nvidia.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mehdi_amini added inline comments.Tue, Nov 2, 10:32 PM
flang/include/flang/Optimizer/Builder/FIRBuilder.h
378

Is there a use for this method? (same for the other above)

384

No definition added for this declaration?

429

Is this method used somewhere? (Same for the two above)

flang/include/flang/Optimizer/Transforms/Factory.h
146

Do you know why this is returning a vector instead of an OperandRange? (this is a question for the ShapeOp definition).

It seems strange to force copy on all the accessors when they may just want to iterate on the Values...

flang/lib/Optimizer/Builder/FIRBuilder.cpp
335–383

Is this method used?

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
674
681
700
795
clementval marked 51 inline comments as done.Mon, Nov 22, 1:05 PM

Thanks for the update, the size makes it tractable now, and the test helps to figure out the corner cases when the code isn't straightforward.

I mostly looked at ArrayCopyAnalysis here.

One thing that can be problematic for future patches: the control flow in general should be handled with the ControlFlowInterface which may not be yet implemented on the relevant FIR ops. But that had a significant impact on the code inside the passes, so likely something to consider prioritizing downstream.

Tried to address most of the comments. I think we will need couple of follow up patches to make it easier to move foward (and sync back in between).

flang/include/flang/Optimizer/Builder/FIRBuilder.h
378

This one is not used currently (removed from this patch). The one above is.

384

Mistake of the rebase. It's the same as the one just above. Removed.

flang/include/flang/Optimizer/Transforms/Factory.h
146

I don't think there is a good reason for this. Will add a TODO so we can address this in a follow up patch since it touches the op definiton and probably other places in the code.

flang/include/flang/Optimizer/Transforms/Passes.td
95

These were artifact from the time it didn't work without it. Looks like it is now fine to remove them.

flang/lib/Optimizer/Builder/FIRBuilder.cpp
289

Yeah will be added later anyway.

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
90

Updated.

121

Would it be ok to address this in a follow up patch? I just added a todo right now.

156

Added some test and removed the IterWhileOp and EmboxOp special case since they will be added later when we can add better test coverage.

235

From a practical point of view, array operations are normally generated with DoLoopOp so there is no real code that need the other loops. From a purely FIR point of view, of course someone could write some IterWhileOp with array operations but we do not have use cases in mind for that.
I added a comment to make precise this in the code.

261

Same here. Would be great if we can address that in a follow up patch directly after finishing this one.

310

They are loops with finalValue and iterArgs in the tests. Will add the documentation in another path. Added on my todo list.

316–320

I think it is better to handle this later (switch to control flow interface) as it will bring lots of changes with it.

361

Thanks for the patch. look like it does the trick.

439–443

Thanks for the inputs here. Replace by a simple walk.

449

ArrayMergeStoreOp verifier has this:

if (!isa<ArrayLoadOp>(op.original().getDefiningOp()))
    return op.emitOpError("operand #0 must be result of a fir.array_load op");
824

Thanks.

I think you haven't updated the revision yet.

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
121

How to we track that this get addressed though? I'm wary of TODO that will stay here for a while.
Is someone gonna commit to work on this ASAP?

clementval marked 14 inline comments as done.

Address various review comments + rebase

I think you haven't updated the revision yet.

Sorry, it was stuck on the arc lint phase :(

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
121

I can work on that in the next 2-3 weeks probably.

Looks fine overall, thanks!

There are still a few minor issues: dead code in particular, but also the code documentation is a bit on the light side for something that really does not seem trivial to me.
(+ some inline comments)

flang/include/flang/Optimizer/Builder/FIRBuilder.h
66

Seems unused in this patch?

343

Unused in this patch?

410

This is unused in this patch?

421

Same

430

Unused as well (and undocumented)

flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
360

Can you document all this macro system?
Does it need to in header? In general macros defined in header is "dangerous", should these be prefixed with Fir or something?

364

You could add this in the function body if this is really needed, or what do I misse?

flang/include/flang/Optimizer/Transforms/Factory.h
45

This method isn't used anywhere either

148

I don't see a call-site for this either

191–193

Move this assert with the one a few lines above? They seem related.
Using the assert(!shapeVal || form like above you can avoid the if and the nesting

254

The createLoopNest functions seem unused as well

flang/lib/Optimizer/Builder/FIRBuilder.cpp
337
343

Seems like there should be a llvm_unreachable after this error is emitted

345

I don't quite find what the .match( call resolves to on the fir::ExtendedValue class? Any tip?

347

(same below)

372

Can you comment on what this is supposed to match?

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
356

not sure if it is a Value here?

454–463

I commented on this before I believe: you have two map lookup when one would be enough I think:

469

You marked it done, but I don't see it done in the code.

645
672

(previous use of useMap.lookup() in this function spelled out the type)

717

This branch may be worth a comment.

In general the entire pattern is a bit light on documentation.

731

You can reduce the indentation in the code by inverting the else and the then branch:

if (!inEleTy.isa<fir::RecordType>())
   llvm::report_fatal_error("not a legal reference type");

Also, I'm not sure why it isn't an assertion here? If this can't be then shouldn't we degrade more gracefully?

assert(inEleTy.isa<fir::RecordType>());
flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
121

I have created an issue to capture this in the upstreaming FIR Passes board.
https://github.com/flang-compiler/f18-llvm-project/projects/7
https://github.com/flang-compiler/f18-llvm-project/issues/1268

@clementval if this is just about converting to an iterative version then we can help.

clementval marked 23 inline comments as done.

Address review comments

flang/lib/Optimizer/Builder/FIRBuilder.cpp
345
flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
121

Yeah this is it. With all the changes (compare to fir-dev) in this patch it would be nice to be able to make a sync before this kind of refactoring.

454–463

Yeah sorry I thought I made the update. Must have been lost in an other branch.

645

Sorry missed this update as well. It's updated.

717

ArrayUpdateOp is actually not supported for reference type anymore since there is a new op for it. This code is removed.

731

Replaced with an assert and remove the if-else

@kiranchandramohan do you plan on reviewing/approving the current version of the patch?

I don't have any more comment than before, there are just a few that are still pending.

flang/include/flang/Optimizer/Builder/FIRBuilder.h
410

Missed this one?

flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
360

Marked done, but I don't see a change here?

flang/lib/Optimizer/Builder/FIRBuilder.cpp
30

This is marked done, but you actually extracted it as-is and landed it in https://reviews.llvm.org/D112057 ; can you address that as well (in a separate patch, but I'm wary of losing track of this).

46

Same here: you marked it done without commenting and landed it separately.

345

Thanks!

Something really confusing is that there is another class with the same name, and located in a header with the same filename!
(in include/flang/Lower/Support/BoxValue.h)

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
469

Yet again
(line 452 by now)

clementval marked 3 inline comments as done.

Address missed comments

clementval added inline comments.Fri, Nov 26, 4:16 AM
flang/include/flang/Optimizer/Builder/FIRBuilder.h
410

My bad. I thought I removed it.

flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
360

No idea why it was mark done since I don't even remember reading this one. I have added some comments and prefixed the macros. I kept mkRTKey without prefix since it is used outside.

flang/lib/Optimizer/Builder/FIRBuilder.cpp
30

Sorry, I marked it as done here to be able to navigate more easily in the comments since it was not part of this part anymore. I'll look at it in another patch.

46

Is this really important? If so I'll update it.

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
469

My bad. I'm sure I made the change but I probably mixed up my branches and it was not in this update. Updated now.

clementval marked an inline comment as done.Fri, Nov 26, 4:17 AM
clementval added inline comments.
flang/lib/Optimizer/Builder/FIRBuilder.cpp
30

@kiranchandramohan do you plan on reviewing/approving the current version of the patch?

I don't have any more comment than before, there are just a few that are still pending.

The patch LGTM. Thanks for going through the patch in detail and pointing out various issues. And thanks to @clementval for addressing most of the issues. I agree that the remaining issues can be dealt with later. I have created a few github issues to follow up on later work.

Nit: The changes in the following files are in other patches or have been merged. How are you planning to handle this?
flang/lib/Optimizer/Dialect/FIRType.cpp
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
flang/include/flang/Optimizer/Builder/Runtime/Assign.h
flang/lib/Optimizer/Builder/Runtime/Assign.cpp

flang/lib/Optimizer/Builder/FIRBuilder.cpp
30
46

Might be easier to read for newcomers.

345

I think there is some refactoring going on, where code in Lower is moved to the Optimizer/Builder directory. I think once all the code is moved the code in Lower can be deleted.

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
121

OK. I have moved this to the post-upstream board.

310
316–320