Page MenuHomePhabricator

[fir] Add fir.extract_value and fir.insert_value conversion
ClosedPublic

Authored by clementval on Mon, Nov 1, 2:01 PM.

Details

Summary

This patch add the conversion pattern for fir.extract_value
and fir.insert_value. fir.extract_value is lowered to llvm.extractvalue
anf fir.insert_value is lowered to llvm.insertvalue.
This patch also adds the type conversion for the BoxType and RecordType
needed to have some comprehensive tests.

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

Diff Detail

Event Timeline

clementval created this revision.Mon, Nov 1, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 1, 2:01 PM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
clementval requested review of this revision.Mon, Nov 1, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 1, 2:01 PM

Overall makes sense to me. I would love to see less auto - in quite a few places it replaces simple types like mlir::Type. I'm rather unfamiliar with this code-base (and MLIR) and it is easier for me to follow the logic when I see the actual type. Especially when translating between dialects (so moving from one namespace to some other namespace).

flang/lib/Optimizer/CodeGen/DescriptorModel.h
21–34

Could comments that apply to a whole file be moved to the top? Like here: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/AffineExpr.h#L7-L12

On a separate note - the TODO suggests that --target is unlikely to work correctly. I think that it would help to have this mentioned somewhere, e.g. in https://github.com/llvm/llvm-project/blob/main/flang/docs/RuntimeDescriptor.md. I know that that's not really part of up-streaming, but it might really limit our options in terms of testing.

116
120

The spec for F2018 (18.5.3) lists only 7 fields. Also, nothing is really checked here, is it? The comment probably needs updating.

flang/lib/Optimizer/CodeGen/TypeConverter.h
65

Why wrap this in a method as opposed to having a constant?

65–66
70

Isn't this just CFI_cdesc_t? I'm just confused by parts here.

143

Shouldn't the result be checked instead?

flang/test/Fir/convert-to-llvm.fir
117
134

Missing llvm.return? (for consistency with other tests)

153

Missing llvm.return? (for consistency with other tests)

166

Missing llvm.return? (for consistency with other tests)

clementval updated this revision to Diff 384584.Wed, Nov 3, 2:24 PM
clementval marked 10 inline comments as done.

Address review comment

clementval added inline comments.Wed, Nov 3, 2:31 PM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
21–34

As the todo mentioned, this will need some work to be done right. This will probably need someone to dig deeper into that.

120

This is actually a compile time check. if you remove on element or add one your build will fail. I don't think we are at F2018 yet.

flang/lib/Optimizer/CodeGen/TypeConverter.h
65

Since it's constexpr does it really matter?

70

There is more than just the 8 fields of the CFI_cdesc_t so it isn't really it so I guess parts or other name could work as well.

mehdi_amini added inline comments.Wed, Nov 3, 6:00 PM
flang/lib/Optimizer/CodeGen/TypeConverter.h
111

That makes it a global mutable map, that seems unsafe to me in general.

Here it seems even worse: this is never re-initialized and so it creates coupling between the lifetime of a Context (and even a module) and the entire process. Am I missing something?

clementval updated this revision to Diff 384692.Thu, Nov 4, 3:07 AM

Review old map cache

clementval marked an inline comment as done.Thu, Nov 4, 3:09 AM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/TypeConverter.h
111

I believe we can get rid of this entirely since LLVMStructType::getIdentified is doing the caching here already. Probably an old artifact when it was not working as expected.
Thanks for pointing this out.

clementval updated this revision to Diff 384695.Thu, Nov 4, 3:28 AM
clementval marked an inline comment as done.

Rebase after couple of other conversion patches have landed

mehdi_amini added inline comments.Thu, Nov 4, 6:31 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
302–307

(this is the canonical/generic way of handling constants)

309

Function isn't used?

flang/lib/Optimizer/CodeGen/TypeConverter.h
61

Is all the code related to the box type conversion coupled to the rest of this patch? I don't see where it is tested and it seems that the tests for fir.extract_value and fir.insert_value lowering aren't involving the box type?

clementval updated this revision to Diff 384994.Fri, Nov 5, 2:27 AM
clementval marked 4 inline comments as done.

Cleanup code after review comments

clementval updated this revision to Diff 384995.Fri, Nov 5, 2:28 AM

Rebase

flang/lib/Optimizer/CodeGen/CodeGen.cpp
309

Not needed anymore. Removed for now.

flang/lib/Optimizer/CodeGen/TypeConverter.h
61

It was in my plan to bring !fir.box conversion along in this patch but in order to test it well it requires to bring along some other code. It's better to do this in another revision focused on that. Remove the code in this patch for now.

awarzynski added inline comments.Fri, Nov 5, 3:53 AM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
21–34

Yup, that's fine, but I would really appreciate a note on that that can be found outside the code itself. And, that can be referred inside the code whenever such assumptions are made.

I've created https://llvm.org/PR52418 for this reason. Would you mind adding a link?

120

I don't think we are at F2018 yet.

No, but this comes from the Fortran_2018 namespace, that's why I referred to the 2018 spec. This is all fine to, but 8 is a magic number here. Why do we check for 8 rather than 7 or 13 fields? Could add any sort of hint that would demystify this magic number?

flang/lib/Optimizer/CodeGen/TypeConverter.h
51

How about a test in "types-to-llvm.fir" for this conversion?

clementval updated this revision to Diff 385050.Fri, Nov 5, 6:19 AM
clementval marked 3 inline comments as done.

Add comments

flang/lib/Optimizer/CodeGen/TypeConverter.h
51

Type conversion is checked alongside op conversions.

awarzynski accepted this revision.Fri, Nov 5, 6:25 AM

Thank you for working on this and addressing my comments! LGTM

This revision is now accepted and ready to land.Fri, Nov 5, 6:25 AM
Meinersbur added inline comments.
flang/lib/Optimizer/CodeGen/DescriptorModel.h
90

This breaks the windows build:

C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\flang\lib\Optimizer\CodeGen\DescriptorModel.h(95): error C2766: explicit specialization; 'fir::TypeBuilderFunc (__cdecl *fir::getModel<__int64>(void))(mlir::MLIRContext *)' has already been defined
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\flang\lib\Optimizer\CodeGen\DescriptorModel.h(70): note: see previous definition of 'getModel'

CFI_index_t is a typedef of ptrdiff_t which is a 64 bit signed integer. That is, the same as getModel<long long> template specialization. Don't know why it works under Linux or why the Windows pre-merge check didn't run.

clementval added inline comments.Fri, Nov 5, 12:51 PM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
90
Meinersbur added inline comments.Fri, Nov 5, 1:01 PM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
90

Thanks, it successfully fixed the build: https://lab.llvm.org/buildbot/#/builders/172/builds/5311

clementval added inline comments.Fri, Nov 5, 2:18 PM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
90

@Meinersbur Removing this is actually making all other buildbots to fail. I reverted this patch until we can figure out what's happening.

Meinersbur added inline comments.Fri, Nov 5, 3:31 PM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
90

Which one failed? I cannot find one at e.g. https://lab.llvm.org/buildbot/#/builders/175

By LLVM policy, this should have been mentioned in the reverting commit.

mehdi_amini added inline comments.Fri, Nov 5, 3:41 PM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
90

On Linux ptrdiff is long : https://godbolt.org/z/fdacb8f7v

What if you replace this specialization with a specialization for long instead? This seems to be the one missing?

Meinersbur added inline comments.Fri, Nov 5, 4:10 PM
flang/lib/Optimizer/CodeGen/DescriptorModel.h
90

Sounds right. In LLP64 (msvc), long is 32 bits, i.e. ptrdiff_tx could not be long. long is the only missing specialization for getModel (ignoring types smaller than int: short, char)

As a side note, I think that you can re-submit this patch without getExtendedDescFieldTypeModel, getDescFieldTypeModel and getModel methods. You can include them in https://reviews.llvm.org/D113288. IIUC.