This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add fircg.ext_embox conversion
ClosedPublic

Authored by clementval on Nov 18 2021, 2:26 AM.

Details

Summary

Convert a fircg.ext_embox operation to LLVM IR dialect.
A fircg.ext_embox is converted to a sequence of operation that
create, allocate if needed, and populate a descriptor.

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 18 2021, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2021, 2:26 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
clementval requested review of this revision.Nov 18 2021, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2021, 2:26 AM

Make test target agnostic

Minor comments and request for two more tests.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1453–1469

Nit: add the name for the pos 7.

1555

Nit: else can be removed by moving the lowerBound pushing outside.

1579

Nit: remove commented code.

1660

Is this case covered in the test? (the char has a dynamic size)

1678

I think we need a test for derived type as well.

clementval marked 5 inline comments as done.

Address review comment

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1678

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

1682

Not sure about this one since it was not a TODO. But please see the following discussion.

https://reviews.llvm.org/D114104#inline-1089507

1787

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

1799

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

clementval marked 4 inline comments as done.Nov 19 2021, 2:23 AM

Put TODO back in place

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1453–1469

updated. Added also some constexpr for the values in [dims].

1660

It is in the second test.

1678

Type conversion make it fails before we can reach the pattern

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1791

I think the last argument is using the original operand and not the converted operand.

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

Nit: Consider this as a nit and I will leave it to you to decide whether we should have it in this patch or should do it later.

  1. A more realistic test case will be helpful here. Consider the following, it has shape, slice and shift operands for the op. Since it has two dimensions, it will exercise the for loop inside the code.
// subroutine sb(n,sh1,sh2)
// integer::n,sh1,sh2
// double precision::arr(sh1:n,sh2:n)
// call xb(arr(2:n,4:n))
// end subroutine
! N is the upperbound, sh1 and sh2 are the shifts or lowerbounds
func @_QPsb(%N: index, %sh1: index, %sh2: index) {
  %c4 = arith.constant 4 : index
  %c1 = arith.constant 1 : index
  %c2 = arith.constant 2 : index
! Calculate nelems in dim1
  %n1_tmp = arith.subi %N, %sh1 : index
  %n1 = arith.addi %n1_tmp, %c1 : index
! Calculate nelems in dim2
  %n2_tmp = arith.subi %N, %sh2 : index
  %n2 = arith.addi %n2_tmp, %c1 : index
  %arr = fir.alloca !fir.array<?x?xf64>, %n1, %n2 {bindc_name = "arr", uniq_name = "_QFsbEarr"}
  %box = fircg.ext_embox %arr(%n1, %n2) origin %sh1, %sh2[%c2, %N, %c1, %c4, %N, %c1] : (!fir.ref<!fir.array<?x?xf64>>, index, index, index, index, index, index, index, index, index, index) -> !fir.box<!fir.array<?x?xf64>>
  fir.call @_QPxb(%box) : (!fir.box<!fir.array<?x?xf64>>) -> ()
  return
}      
func private @_QPxb(!fir.box<!fir.array<?x?xf64>>)
  1. A test with a subcomponent if possible
1581

Does substr have a meaning here since it applies to chars?

clementval marked an inline comment as done.

Address review comments

clementval marked an inline comment as done.Nov 30 2021, 2:26 AM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1791

Not sure I follow. xbox.substr()[0] has no conversion before this use.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1791

I meant xbox.substr()[0] is the original substr operand of the XEmbox operation and not the converted operand. In conversion patterns we should use the converted operands. So, shouldn't we access the converted operand using something like operands[xbox.substrOffset()]?

Use converted operand

clementval marked 2 inline comments as done.Nov 30 2021, 5:08 AM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1791

Ah ok! Makes perfect sens. I was looking for a fir.convert or smth similar and I didn't even think about operand conversion.

clementval marked an inline comment as done.Dec 2 2021, 9:30 AM

ping

kiranchandramohan accepted this revision.Dec 3 2021, 1:20 AM

LGTM.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1658

Nit: mlir::Location

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

Nit: Did you miss this point or is having substr here OK?

This revision is now accepted and ready to land.Dec 3 2021, 1:20 AM
clementval marked 2 inline comments as done.Dec 3 2021, 2:11 AM
clementval added inline comments.
flang/test/Fir/convert-to-llvm.fir
1581

Sorry I thought I did update that. Done.

clementval updated this revision to Diff 391587.Dec 3 2021, 2:11 AM
clementval marked an inline comment as done.

Address last comments

This revision was automatically updated to reflect the committed changes.