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
2981

replace auto

2986

replace auto

3010

replace auto

3013

replace auto

3132
3152
3155
3181
3185

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

3010
3011
3157
3157

Maybe adding braces here would help readability.

3184
3218

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
2981

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

2982

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
2980

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).

3003

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?

3009

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)

3134

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

3139

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

3142

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 :)

3144

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

3188

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

3234

Error?

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

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

2469

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
2980

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?

3003

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?

3009

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

3134

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

3139

Good point, ta!

3142

Let me update the message then :)

3144

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
3188

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.

3234

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>()?

2977

Same as above.

3003

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

3009

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
2511
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!