This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by awarzynski on Nov 18 2021, 6:08 AM.

Details

Summary

This patch extends the FIRToLLVMLowering pass in Flang by adding a
hook to transform fir.coordinate_of into a sequence of LLVM MLIR
instructions.

The following cases are currently supported:

  1. the input object is a fir.complex (wrapped in e.g. fir.ref or fir.box)
  2. the input object is wrapped in a fir.box (including e.g. fir.array).

Note that fir.complex wrapped in a fir.box falls under case 1. above
(i.e. it's a special case regardless of the wrapping type). Sequence
types are not yet supported (e.g. fir.array that is _not wrapped_ with
fir.box).

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

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

awarzynski created this revision.Nov 18 2021, 6:08 AM
awarzynski requested review of this revision.Nov 18 2021, 6:08 AM
awarzynski added inline comments.Nov 18 2021, 6:11 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2316

PLEASE SKIP THIS BLOCK

@rovka will send a separate patch for LenParamIndexOpConversion. I've only added it here to be able to test this.

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

I'll fix these trailing white-spaces in a separate patch and then rebase.

A few nit comments.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2168

Nit: use NotifyMatchfailure

2194

Nit: Notifymatch failure

2292

Nit: remove auto?

2297

Nit: remove auto?

clementval added inline comments.Nov 18 2021, 6:41 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
26

Is this include needed? Can you use "?

200

Replace 0 with kAddrPosInBox

237–239
2159
2188
2234
awarzynski marked 5 inline comments as done.

Address PR comments (remove auto, replace "magic numbers" with variables, switch to notifyMatchFailure in place of mlir::emitError)

awarzynski added inline comments.Nov 18 2021, 9:11 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
26

Is this include needed?

Yes, one of my plugins adds these whenever I switch from auto to a specific type. But this will be included by some other file anyway. I'll just remove it.

200

Thanks!

237–239

Thanks!

2168

Makes sense, but then we won't be able to check for this diagnostic in "convert-to-llvm-invalid.fir". I'm starting to think that we should --debug there. @clementval , what are your thoughts?

schweitz added inline comments.Nov 22 2021, 3:53 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2165

Are we adding backquotes to error messages now? Why?

awarzynski added inline comments.Nov 23 2021, 7:44 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2165

That's a typo, I must have worked on a project in which that's the preferred convention. #too-many-coding-styles. Let me fix this. Thanks for pointing it out!

Fix comments (remove back-ticks)

Refine variable names (e.g. objectTy --> boxedObjTy)

rovka added inline comments.Nov 24 2021, 1:09 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
71

Is this actually used?

2249

Nit: I think you can flip this function around (i.e. check the preconditions first and have the bulk of the code outside of the ifs).
Also, I'm almost afraid to suggest this, but I think the unreachable can be replaced with an assert.

2296

Not saying it should be part of this patch, but is there a test for this somewhere? (I'd expect it in test/Fir/invalid.fir, but AFAICT there isn't anything there)

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

Would be nice to also have a testcase with more than one index operand.

1634
1635
1638
1640
1654
1685
awarzynski marked 6 inline comments as done.

Refactor getIntValue, delete voidPtrTy, add a testcase with more than one index operand, fix typos

awarzynski marked 6 inline comments as done.Nov 24 2021, 11:36 AM
awarzynski added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
71

Nope. Good catch, thanks!

2249

Thanks for the suggestions! Let me rewrite this. Stay tuned!

2296

Good catch, thanks! This is currently not being captured (so can't a test in "invalid.fir"). I'll take a look later and see whether I can implement a separate verifier for this. Probably in a separate patch.

awarzynski added inline comments.Nov 24 2021, 11:40 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2249

Please take a look again.

rovka added inline comments.Nov 25 2021, 12:08 AM
flang/test/Fir/convert-to-llvm.fir
1668

This is very fishy, are you sure this is supposed to work? I'm following up to SUBOBJECT_1_ADDR, IIUC that's the address of field_1. It seems very bizarre to then take this ptr<i32> and cast it to <ptr<struct<"derived_2">> and index into it again. I feel like this should be an error, i.e. the thing that the second index is indexing into should be another struct or array or something, not an i32. Am I misunderstanding the coordinate_of semantics?

awarzynski added inline comments.Nov 25 2021, 3:52 AM
flang/test/Fir/convert-to-llvm.fir
1668

This is very fishy

Agreed. Thanks for catching this and apologies for not paying more intention when submitting last night. I think that we should be getting the following instead (might be easier to parse without the regex):

llvm.func @coordinate_box_derived_2(%arg0: !llvm.ptr<struct<(ptr<struct<"derived_2", (i32, i32)>>, i64, i32, i8, i8, i8, i8, ptr<i8>, array<1 x i64>)>>) {
  %0 = llvm.mlir.constant(0 : i32) : i32
  %1 = llvm.mlir.constant(1 : i32) : i32
  %2 = llvm.mlir.constant(0 : i32) : i32
  %3 = llvm.mlir.constant(0 : i32) : i32
  %4 = llvm.getelementptr %arg0[%2, %3] : (!llvm.ptr<struct<(ptr<struct<"derived_2", (i32, i32)>>, i64, i32, i8, i8, i8, i8, ptr<i8>, array<1 x i64>)>>, i32, i32) -> !llvm.ptr<ptr<struct<"derived_2", (i32, i32)>>>
  %5 = llvm.load %4 : !llvm.ptr<ptr<struct<"derived_2", (i32, i32)>>>
  %6 = llvm.mlir.constant(0 : i64) : i64
  %7 = llvm.getelementptr %5[%6, %0] : (!llvm.ptr<struct<"derived_2", (i32, i32)>>, i64, i32) -> !llvm.ptr<i32>
  %8 = llvm.getelementptr %5[%6, %1] : (!llvm.ptr<struct<"derived_2", (i32, i32)>>, i64, i32) -> !llvm.ptr<i32>
  %9 = llvm.bitcast %5 : !llvm.ptr<struct<"derived_2", (i32, i32)>> to !llvm.ptr<i32>
  llvm.return
}

Does it make sense? I'll send an updated patch shortly. It gets rid of quite a few redundant casts. Unless I misunderstood GEP again and they are actually needed :)

Remove redundant casts

Fix formatting

rovka added inline comments.Nov 29 2021, 2:17 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2326

This still doesn't seem quite right. How are all these GEPs used? It seems you're just creating them, but then you're replacing the original coor with objectBaseAddr, so you're just going around the GEPs (i.e. the users of the coor will get objectBaseAddr instead of a GEP based on objectBaseAddr).

clementval added inline comments.Nov 29 2021, 4:48 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2326

Please double check in fir-dev when you change code like that.

awarzynski added inline comments.Nov 29 2021, 10:31 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2326

@rovka

Good point, thanks! Also, your comment made me realise that I misunderstood how fir.coordinate_of is meant to work for fir.type and fir.array. In particular, fir.type could contain fir.array and vice versa. This is catered for on fir-dev (see here ), but was incorrectly split into "orthogonal" cases by me in this patch. I will revert that and restore one big case for fir.type/fir.array instead.

And yes, this "rewrite" should replace the original OP with a GEP. In theory :) There's a bunch of bitcasts too as fir.array and fir.type cases are a bit different. So, ultimately, it might be an llvm.bitcast applied to a result of llvm.getelemenptr.

Please double check in fir-dev when you change code like that.

Checked, all good 👍🏻 !

Fix how fir.array and fir.type (embedded inside fir.box) are handled. In particular, make sure that fir.type inside fir.array and fir.array inside fir.type are supported.

Thanks @rovka for all the suggestions!

clementval added inline comments.Nov 29 2021, 11:05 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2326

Looks like you are making substantial change from the original code you are upstreaming. Might be wise to submit those change first in fir-dev to be reviewed and test with more extensive tests.

Move one definition of a constant 0 (c0) so that the code generated here and on fir-dev are identical.

awarzynski added inline comments.Nov 30 2021, 2:09 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2326

Might be wise to submit those change first in fir-dev to be reviewed

I appreciate that you are concerned about the delta between main and fir-dev, but the code generated here and on fir-dev is identical (I've just pushed a small change to make sure that this condition is met). The biggest difference are the tests - these will be as "new" on fir-dev as they are here.

I'd like us to finish reviewing these changes here rather than on fir-dev. This is more inclusive (AFAIK, everyone from fir-dev is active here, but not everyone from main visits fir-dev). Perhaps sending a patch with tests added here to fir-dev would be a good idea? The actual changes to CoordinateOpConversion would be sent later as a separate patch (i.e. as a "sync-up" patch). This way fir-dev developers can be confident that the changes introduced in main don't break the code. WDYT?

clementval added inline comments.Nov 30 2021, 2:29 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2326

If you are confident that your change will not trigger a regression on fir-dev then fine for me. The change seems bigger than it really is. I was seeing something about fixing this conversion so I was just worried about what had to be fixed.

clementval added inline comments.Dec 1 2021, 1:41 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2315

Can you fix the clang-format here?

2336

format

rovka added inline comments.Dec 1 2021, 3:09 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2294

Are these really the only 4 cases? Is it possible to have any combination of arrays/deriveds inside of arrays/deriveds, e.g. fir.array<M x fir.type<d1{field1:fir.array<N x fir.array <10 x i32>>, field_2:fir.type<d2{i32, i32}>}>>? Maybe a more generic sentence would work better here

2314

So, IIUC, if you have coordinate_of on a fir.array<M x N x fir.array<P x i32>> with operands idx1, idx2, idx3, then you end up multiplying idx1 * stride(operands[0], 0), then idx2 * stride(operands[0], 1), then idx3 * stride(operands[0], 0). That sounds like you expect the same stride for the inner array as for the outer array. That doesn't seem right. Am I missing something?

Also, is it even possible to have an inner fir.array without a box? Or would you actually have fir.array<M x N x fir.box<fir.array<P x i32>>?

awarzynski marked an inline comment as done.Dec 1 2021, 9:10 AM
awarzynski added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
2294

Let me just mix 2.3 and 2.4 then.

2314

That doesn't seem right. Am I missing something?

I'll double-check with @schweitz. IIUC, it's either a bug or an indication that this fir.box contains one of: !fir.array<!fir.derived<>> or !fir.derived<>.

Also, is it even possible to have an inner fir.array without a box? Or would you actually have fir.array<M x N x fir.box<fir.array<P x i32>>?

Well, there's fir::emitFatalError(loc, "unexpected type in coordinate_of"); towards the end of this method which is hit if the requested element inside the original fir.array or fir.type is neither ... fir.array nor fir.type.

2315

Yup, sorry about that! Still haven't figured why arc lint does nothing for me.

awarzynski updated this revision to Diff 391055.Dec 1 2021, 9:10 AM

Added two more cases, fixed formatting

NOTE: The following discussion refers to the code from doRewriteBox.

I'm looking at the "GENERAL CASE" in doRewriteBox again, which is the trickiest one here. Let me summarise a bit. There are quite a few potential cases and I don't want to miss anything.

The following cases are IMO quite clear and already fully tested:

// 1. (`fir.array`)
%box = ... : !fix.box<!fir.array<?xU>>
%idx = ... : index
%resultAddr = coordinate_of %box, %idx : !fir.ref<U>

// 2 (`fir.derived`)
%box = ... : !fix.box<!fir.type<derived_type{field_1:i32}>>
%idx = ... : i32
%resultAddr = coordinate_of %box, %idx : !fir.ref<i32>

// 3 (`fir.derived` inside `fir.array`)
%box = ... : !fir.box<!fir.array<10 x !fir.type<derived_1{field_1:f32, field_2:f32}>>>
%idx1 = ... : index
%idx2 = ... : i32
%resultAddr = coordinate_of %box, %idx1, %idx2 : !fir.ref<f32>

I've also added a test for the following case, but I am realising that the generated code is not valid:

4. (`fir.array` inside `fir.derived`)
%box = ... : !fix.box<!fir.type<derived_type{field_1:!fir.array<10xf32>}>>
%idx1 = ... : i32
%idx2 = ... : index
%resultAddr = coordinate_of %box, %idx1, %idx2 : !fir.ref<f32>

It's not valid, because the stride is calculated as (@rovka, thanks for pointing this out!):

mlir::Value stride = loadStrideFromBox(loc, operands[0], index - i, rewriter);

i.e., it's always extracted from the outer box (%box in the snippet above). Making this code work correctly for 4. would require a design discussion. I think that's out-of-scope here. Instead, I'll add a new TODO for nested arrays and move the corresponding test to flang/test/Fir/Todo/. WDYT? Also, have I missed any cases?

awarzynski updated this revision to Diff 391291.Dec 2 2021, 5:43 AM

Treat cases with nested arrays (e.g. fix.box<!fir.type<derived_type{field_1:!fir.array<10xf32>}>>) as a TODO. Rebase on top of main.

awarzynski updated this revision to Diff 391292.Dec 2 2021, 5:45 AM

Revert the change in flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h (added by accident)

Switch from to arith.constant to !fir.field in tests. Otherwise, minor improvements.

rovka accepted this revision.Dec 3 2021, 5:48 AM

LGTM, thanks for all the changes :)

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2328

Microscopic nit: recTy would be more suggestive.

flang/test/Fir/Todo/cordinate_of_2.fir
8 ↗(On Diff #391378)

Very small nit: Maybe also have a testcase with derived<derived<array, ...>, ...> ?

flang/test/Fir/convert-to-llvm.fir
1665
This revision is now accepted and ready to land.Dec 3 2021, 5:48 AM
awarzynski updated this revision to Diff 392335.Dec 7 2021, 3:34 AM

Allow dynamically sized arrays inside !fir.box.

I mistakenly disallowed dynamically sized arrays earlier. However, IIUC, boxed arrays (and that's the only type of arrays dealt with in this patch) shouldn't require size when using fir.coordinate_of. In fact, !fir.box<!fir.array<? x i32>> should be no different to !fir.box<!fir.array<? x i32>> in this case. Updated accordingly. @rovka, could you take one final look?

Also rebased on top of main.

awarzynski updated this revision to Diff 392337.Dec 7 2021, 3:43 AM

Address [nits] from @rovka

rovka added a comment.Dec 9 2021, 1:32 AM

Seems fine, thanks.

Allow dynamically sized arrays inside !fir.box.

I mistakenly disallowed dynamically sized arrays earlier. However, IIUC, boxed arrays (and that's the only type of arrays dealt with in this patch) shouldn't require size when using fir.coordinate_of. In fact, !fir.box<!fir.array<? x i32>> should be no different to !fir.box<!fir.array<? x i32>> in this case.

I assume you mean !fir.ref<!fir.array<? x i32>> versus !fir.box<!fir.array<? x i32>> :)

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

Do we still expect an error here?

Seems fine, thanks.

Allow dynamically sized arrays inside !fir.box.

I mistakenly disallowed dynamically sized arrays earlier. However, IIUC, boxed arrays (and that's the only type of arrays dealt with in this patch) shouldn't require size when using fir.coordinate_of. In fact, !fir.box<!fir.array<? x i32>> should be no different to !fir.box<!fir.array<? x i32>> in this case.

I assume you mean !fir.ref<!fir.array<? x i32>> versus !fir.box<!fir.array<? x i32>> :)

I meant that !fir.box<!fir.array<10 x i32>> and !fir.box<!fir.array<? x i32>> should be identical :) That was the source of confusion for me initially and it finally clicked when working on https://reviews.llvm.org/D115333. Basically, I thought that !fir.box<!fir.array<? x i32>> shouldn't work and that's why I added the extra check in doRewrite. However, since !fir.array is "boxed" (that's the only case considered here), this should be just fine and so I removed that check.

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

This particular line was copied by mistake, sorry! It will be ignored as we don't use -verify-diagnostics in this file.

Instead, the code is correctly converted (see the expected output below).

awarzynski updated this revision to Diff 393085.Dec 9 2021, 2:42 AM

Remove an out-of-date test check (this was copied earlier by accident)

Thank you all for reviewing! I'll merge this tomorrow if there are no new comments.

This revision was landed with ongoing or failed builds.Dec 10 2021, 12:35 AM
This revision was automatically updated to reflect the committed changes.