This is an archive of the discontinued LLVM Phabricator instance.

[flang][codegen] Add a conversion for `fir.coordinate_of` - part 2
ClosedPublic

Authored by awarzynski on Dec 8 2021, 5:03 AM.

Details

Summary

This patch extends the FIRToLLVMLowering pass in Flang by extending
the hook to transform fir.coordinate_of into a sequence of LLVM MLIR
instructions (i.e. CoordinateOpConversion::doRewrite). The following
case is added:

  1. the input object is inside !fir.ref (e.g. !fir.ref<!fir.array> or !fir.ref<!fir.derived>).

This is part of the upstreaming effort from the fir-dev branch in [1].

[1] https://github.com/flang-compiler/f18-llvm-project

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

Depends on: D114159

Diff Detail

Event Timeline

awarzynski created this revision.Dec 8 2021, 5:03 AM
awarzynski requested review of this revision.Dec 8 2021, 5:03 AM
clementval added inline comments.Dec 8 2021, 10:22 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2982

replace auto

2987

replace auto

3011

replace auto

3014

replace auto

3134
3154
3157
3183
3187

auto

clementval added inline comments.Dec 9 2021, 7:57 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2943

Remove brace on simple single statement

2947

Remove brace on simple single statement

3011
3012
3159
3159

Maybe adding braces here would help readability.

3186
3220

Remove braces

awarzynski marked 17 inline comments as done.Dec 10 2021, 1:19 AM

Address auto and formatting comments

clementval added inline comments.Dec 10 2021, 1:37 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2982

we use std::size_t in other places. Can you add it here as well and in other places?

2983

Replace size_t with std::size_t

rovka added a comment.Dec 10 2021, 2:44 AM

Love the tests! I think the code allows more cases though, e.g. array of array, array inside derived etc. I think we should have lots of mixed testcases like that, since the code is particularly hard to follow :-/ Or if we don't support that we should consider erroring out early (sorry if we already do and I just missed it).

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2981

Why is this here and not in the verifier? Or is this just the subset of coordinate stuff that's supported at the moment in the CodeGen? If so, then maybe we should call it supportedCoordinate instead and add a comment that we intend to support more in the future (and that eventually the whole function should disappear once everything is supported).

3004

The i >= sz part is strange. Unless I'm missing something, that can only happen in the case of a sequential type, but the docs say "When computing the coordinate of an array element, the rank of the array must be known and the number of indexing expressions must equal the rank of the array". IIUC that means i has to be exactly equal to sz, no?

3010

Same as above, shouldn't this be in the verify function? (There seems to be something along these lines, but it doesn't look very complete)

3136

Nit: Should we also support mlir::TupleType, like we do in hasSubDimensions?

3141

Nit: You could just have bool columnIsDeferred = !hasSubdimension;

3144

If it's incorrect, then it should be an error rather than a TODO. If it's just unsupported yet, then I think the message should say that instead :)

3146

Nit: This comment is superfluous (and ungrammatical), I think you can skip it. hasKnownShape is clear enough imo.

3190

Is this really a TODO? Sounds more like an error...

3236

Error?

flang/test/Fir/convert-to-llvm.fir
2420

Nit: I think it would be worthwhile to introduce a %arg2 here

2432

Nit: I think you can skip the definition for derived_4 on the other 2 lines.

awarzynski marked 6 inline comments as done.Dec 15 2021, 3:42 AM
awarzynski added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2981

Why is this here and not in the verifier? Or is this just the subset of coordinate stuff that's supported at the moment in the CodeGen?

If we move this to the verifier then we will effectively change the semantics of !fir.coordinate_of. That's something that should probably be discussed first. I did experiment with this idea and saw quite a few new regressions, so clearly other parts of the compiler do generate code that this method deems as unsupported.

IIUC, the purpose of this method is to capture cases not supported by this conversion rather then to refine the semantics of !fir.coordinate_of. I will rename it and add some comments to clarify this. I do feel that some cases captured here are genuinely invalid and should indeed be rejected by the verifier, but we'd probably have to do on a case-by-case basis. How about a TODO for now?

3004

The i >= sz part is strange. Unless I'm missing something, that can only happen in the case of a sequential type, but the docs say "When computing the coordinate of an array element, the rank of the array must be known and the number of indexing expressions must equal the rank of the array". IIUC that means i has to be exactly equal to sz, no?

Thanks for the link! Yeah, I agree that this is confusing. See this example. It's inconsistent with the documentation that you referred to in your comment. Perhaps the docs should be updated to read "(...) must not be greater than the rank of the array." instead of "(...) must equal the rank of the array.". WDTY?

3010

Hm, but both true and false might lead to a valid case. Unless I missed something.

3136

We should. In fact, this assert is not needed here. FIROps.cpp takes care of this. I will remove this.

3141

Good point, ta!

3144

Let me update the message then :)

3146

Indeed, thanks!

awarzynski marked 3 inline comments as done.
  • Renamed validCoordinate as supportedCoordinate (also added comments, renamed variables)
  • Added a test for fir.char and tuple
  • Added tests for TODOs and diagnostics
  • Removed an assert that was duplicating a similar assert in FIROps.cpp
awarzynski added inline comments.Dec 15 2021, 8:28 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3190

Agree. I was trying to come up with an example that would get me here, but all of my invalid examples are detected earlier on.

3236

Probably. I was trying to come up with an example that would get me here, but all of my invalid examples are detected earlier on.

  • Replace a couple of TODOs with an error
  • Make sure that !fir.ptr is supported too (alongside !fir.ref)
rovka accepted this revision.Dec 16 2021, 4:35 AM
  • Replace a couple of TODOs with an error
  • Make sure that !fir.ptr is supported too (alongside !fir.ref)

Is there a test for fir.ptr?

Other than that, LGTM (with microscopic nits).

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2949

Nit: I'm not 100% sure, but can you use baseObjectTy.isa<fir::ReferenceType, fir::PointerType>()?

2978

Same as above.

3004

Ok, I was not aware of that example :) Let's update the docs then.
Also, thanks for the sz->numOfCoors, that's more readable.

3010

Yes, sorry, big patch, I think I had paged out some parts of it by the time I was done reviewing :D

flang/test/Fir/convert-to-llvm.fir
2474
This revision is now accepted and ready to land.Dec 16 2021, 4:35 AM
awarzynski marked 2 inline comments as done.

Address the final comments from @rovka. Renamed doRewriteRef as doRewriteRefOrPtr.

Is there a test for fir.ptr?

Added in the latest update :) As fir.ptr and fir.ref are practically identical, I've only added one sanity-test for fir.ptr (to avoid duplication). If there are no further comments, I'll merge later this week.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2949

That's a neat trick, thanks!