This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add fircg.ext_array_coor conversion
ClosedPublic

Authored by clementval on Nov 16 2021, 12:34 AM.

Details

Summary

This patch adds the conversion pattern for the fircg.ext_array_coor
operation. It applies the address arithmetic on a dynamically shaped, shifted
and/or sliced array.

This patch is part of the upstreaming effort from fir-dev branch.

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

Diff Detail

Event Timeline

clementval created this revision.Nov 16 2021, 12:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 12:34 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
clementval requested review of this revision.Nov 16 2021, 12:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 12:34 AM

Remove shape, shapeshift, sshift and slice conversion pattern that are not part of this patch

Some minor comments.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
126–127

Does 0 and 7 have a name defined elsewhere?

172–192

If this is common code with embox then it might be better to add this patch as a child patch of embox.

1374

Nit: Is something missing before etc?

1460

Can we do an early return here and remove the following else?

1467

Notify failure.

1501

Notify failure.

clementval marked 5 inline comments as done.

Address comment + rebase

clementval added inline comments.Nov 17 2021, 9:07 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
126–127

7 has one. 0 is the base for GEP.

172–192

I'll rebase once fir.embox is in.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1471

Please replace back with TODO.
See discussion in https://reviews.llvm.org/D114104#inline-1089507

1506

Please replace back with TODO.
See discussion in https://reviews.llvm.org/D114104#inline-1089507

clementval marked 3 inline comments as done.

Replace TODO + rebase

I have some Nit comments.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1391–1394

Nit: Since these are all for the for loop, would it be better to initialize it in the loop init section?

1481

Nit: off or offset?

1490
1491

Nit: I think we need a test for this case.

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

Nit: I think we need a test with a box.

1361

Nit: Not used?

1369

Nit: Can we have a non-zero shift and slice? Also can the slice be of a different dimension (3) than the array (1)? Or have I misunderstood?

1384

Nit: Not used?

clementval marked 7 inline comments as done.

Address review comments

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1391–1394

This doesn't work AFAIK or am I missing something?

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

The slice for a rank has 3 operands (lower, upper and step).

I have commented inline about places where we seem to be using the original operands. Please have a look and if required, switch to using the converted operands.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
129

Nit: 2 -> kDimStridePos ?

1391–1394

OK. Can you move it closer (just above) to the for loop?

1407–1409

I think the indexOps and shiftOps are the original operands. We have to use the converted operands here I think.

1424

Here also use converted.

1442

Use converted in place of shapeOps.

1524

This should be the converted operands and not the original.

clementval marked 3 inline comments as done.Dec 6 2021, 5:00 AM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1407–1409

I don't think it would make a difference here in the end and probably harder to read to code with operands and indexing. WDYT?

clementval updated this revision to Diff 392027.Dec 6 2021, 5:02 AM

Address some of the review comments

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1407–1409

If this is causing no issues now then it is OK with me. It might cause an issue in the future. Can you either add a TODO or create a ticket to clean it up in the future?

For the XReboxOp I had to use the converted operands to make it work.
https://reviews.llvm.org/D114709

clementval added inline comments.Dec 6 2021, 5:07 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1407–1409

I'm just gonna update it then.

clementval updated this revision to Diff 392039.Dec 6 2021, 6:16 AM
clementval marked 3 inline comments as done.

Use converted operands

clementval updated this revision to Diff 392043.Dec 6 2021, 6:20 AM
clementval marked an inline comment as done.

Use converted operands

clementval marked an inline comment as done.Dec 6 2021, 6:20 AM
This revision is now accepted and ready to land.Dec 6 2021, 6:22 AM
This revision was automatically updated to reflect the committed changes.