This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add !fir.len_param_index conversion
ClosedPublic

Authored by rovka on Nov 19 2021, 6:02 AM.

Details

Summary

Convert len_param_index into a constant. The patch contains a lot of
FIXMEs and very likely computes the wrong value. I have a few questions
about the whole thing but I'll add them as comments so it's easier to follow
the discussion.

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

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

Diff Detail

Event Timeline

rovka created this revision.Nov 19 2021, 6:02 AM
rovka requested review of this revision.Nov 19 2021, 6:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2021, 6:02 AM
rovka added inline comments.Nov 19 2021, 6:09 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
852

I'm not entirely sure what we're supposed to compute here. My understanding is that we have a CFI_cdesc_t with an addendum right after it, and the addendum looks like DescriptorAddendum.
So the offset we're trying to compute needs to skip over the whole CFI_cdesc_t and then the derivedType_ member, and potentially other LEN parameters that come first in len_.
Is this correct?
Also, what's this TODO about?

flang/test/Fir/convert-to-llvm-target.fir
162 ↗(On Diff #388482)

Why does it work to use the names of fields? I would expect it to blow up if we pass something that isn't a len param.

jeanPerier added inline comments.Nov 19 2021, 6:39 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
852

I'm not entirely sure what we're supposed to compute here.

Not an expert on this op (this is not used yet in lowering since it deals with >= F2003 features). It looks like this is computing the offset in bytes (from the descriptor base address) where the length parameter lies in memory.

The TODO is related to the fact that its current codegen always returns the first length parameter offset, instead of actually looking for the one that is queried, so it is clearly wrong and would probably deserve a hard TODO.

The FIROps.td description tells this op can be combined with a fir.coordinate_of to retrieve the length parameters address.

It looks very early design to me, and we should probably re-think the approach here, I am not sure we need an op that gives us the length parameters address, I think we will most likely always want to query the value from a fir.box. The length parameters will most likely be written into the descriptor by the runtime / fir.embox or fir.rebox. But this is design that need to be discuss first.

So I am not sure what to say here, maybe a hard TODO in the codegen part is the way to go for now ?

flang/test/Fir/convert-to-llvm-target.fir
162 ↗(On Diff #388482)

I agree the verifier should complain, it makes no sense to not use a len parameter name.

schweitz added inline comments.Nov 19 2021, 8:52 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
852

LEN (variable) type parameters may be stored in the descriptor of an object or elsewhere.

In the FIR model, they are a special set of integral members in an instance of a derived type. They behave like ordinary data members and can be addressed and accessed.

The reason for the FIXME is that lowering will require more infrastructure work before it can correctly access the requested variable type parameter.

flang/test/Fir/convert-to-llvm-target.fir
162 ↗(On Diff #388482)

It should _not_ accept field names.

It sounds like this Op is incomplete/incorrect and not needed. I would be in favor of turning this into a TODO - otherwise it's just too confusing. But it will only makes sense if we do the same on fir-dev. WDYT?

flang/test/Fir/convert-to-llvm-target.fir
162 ↗(On Diff #388482)
rovka added a comment.Nov 22 2021, 4:05 AM

It sounds like this Op is incomplete/incorrect and not needed. I would be in favor of turning this into a TODO - otherwise it's just too confusing. But it will only makes sense if we do the same on fir-dev. WDYT?

SGTM.

I'll send a separate patch for the verifier/docs.

rovka updated this revision to Diff 388870.Nov 22 2021, 4:06 AM

Bail out until we finalize the design.

rovka added inline comments.Nov 22 2021, 4:09 AM
flang/test/Fir/convert-to-llvm-target.fir
162 ↗(On Diff #388482)

Actually I think the example is correct, that looks like an empty struct with just a LEN param. LEN params are in parentheses, fields are between braces.

Thanks, LGTM! Could you wait for @schweitz or @jeanPerier to have a look before landing this?

flang/test/Fir/convert-to-llvm-target.fir
162 ↗(On Diff #388482)

Oh, that's a very fine difference, thanks for pointing this out :)

jeanPerier accepted this revision.Nov 23 2021, 4:06 AM

LGTM, thanks

This revision is now accepted and ready to land.Nov 23 2021, 4:06 AM
This revision was automatically updated to reflect the committed changes.