This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add fir reduction builder
ClosedPublic

Authored by clementval on Nov 23 2021, 10:53 AM.

Details

Summary

This patch introduces a bunch of builder functions
to create function calls to runtime reduction functions.

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

Co-authored-by: Jean Perier <jperier@nvidia.com>
Co-authored-by: mleair <leairmark@gmail.com>

Diff Detail

Event Timeline

clementval created this revision.Nov 23 2021, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 10:53 AM
clementval requested review of this revision.Nov 23 2021, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 10:53 AM
clementval edited the summary of this revision. (Show Details)Nov 23 2021, 11:15 AM

Extract base of test to be used by other

rovka added a comment.Nov 24 2021, 2:40 AM

Just some initial comments (haven't managed to look through everything yet).

flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
31
62

Can we get rid of all of this duplication by using std::is_integral etc?

flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

Should we use backticks or something around function names? It reads really strange otherwise.
Also, isn't this actually AllDim?

flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
57

If RuntimeCalltestBase can be factored out into a separate patch, the other patches can be tested or reviewed independently.

awarzynski added inline comments.Nov 24 2021, 9:07 AM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

Should we use backticks or something around function names?

+1

Also, what is the difference between genAllDescriptor and genAll? Why do we need both?

flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
15
21

Same suggestion in other places.

26–27

Move next to other "// Commonly used types" in RuntimeCallTest?

flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h
21
87–93

If RuntimeCalltestBase can be factored out into a separate patch, the other patches can be tested or reviewed independently.

I second that. I appreciate that there's a lot of repetition here, but I would really help if this was split into smaller patches.

If RuntimeCalltestBase can be factored out into a separate patch, the other patches can be tested or reviewed independently.

Done in D114557. I'm going to rebase other patch on this.

If RuntimeCalltestBase can be factored out into a separate patch, the other patches can be tested or reviewed independently.

Should I move flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h as well to the initial patch? It would makes sense since all other need this file.

flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

The comments are pretty clear on the differences.

rovka added inline comments.Nov 25 2021, 12:13 AM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

But to clarify: the runtime sometimes has different entry points for the same intrinsic, for special cases. In this case, IIUC, there's All and AllDim, both of which implement the ALL intrinsic. If you look in the standard, you'll see there's 2 variants: ALL(MASK) or ALL(MASK, DIM).

If RuntimeCalltestBase can be factored out into a separate patch, the other patches can be tested or reviewed independently.

Should I move flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h as well to the initial patch? It would makes sense since all other need this file.

Yes, that would be good. You can cite other patches which are consumers.

awarzynski added inline comments.Nov 25 2021, 1:57 AM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

(...) to clarify (...)

Many thanks @rovka !

If you look in the standard (...)

Ah, I see! In fact, I think that it would make a lot of sense to refer to the standard here and to use the notation from there as well (like in Diana's comment).

The comments are pretty clear on the differences.

I disagree. There is genAllDescriptor and genAll, but there's no reference to any "descriptor" in the comments, arguments or the implementation. Also, both methods seem to "Generate call to all runtime routine", but one returns an mlir::Value, whereas the other is void. If both basically generate a call, then why are the interfaces completely different? I appreciate the this implementation may depend on the APIs in the runtime, but it still does not explain what this "descriptor" in genAllDescriptor is and what are the return values are.

flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

For the descriptor version, the result is in an array for which a descriptor is needed. It is probably created by the runtime.

clementval added inline comments.Nov 25 2021, 2:28 AM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

I disagree. There is genAllDescriptor and genAll, but there's no reference to any "descriptor" in the comments, arguments or the implementation. Also, both methods seem to "Generate call to all runtime routine", but one returns an mlir::Value, whereas the other is void. If both basically generate a call, then why are the interfaces completely different? I appreciate the this implementation may depend on the APIs in the runtime, but it still does not explain what this "descriptor" in genAllDescriptor is and what are the return values are.

There is a reference to descriptor in the comment and there is a result argument which is the "descriptor".

clementval marked an inline comment as done.Nov 25 2021, 3:39 AM
clementval added inline comments.
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
62

Not sure I see where do you want us to go here. Do you have an example?

If RuntimeCalltestBase can be factored out into a separate patch, the other patches can be tested or reviewed independently.

Should I move flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h as well to the initial patch? It would makes sense since all other need this file.

Yes, that would be good. You can cite other patches which are consumers.

RTBuilder.h moved to D114557

clementval marked 4 inline comments as done.Nov 25 2021, 5:16 AM
clementval added inline comments.
flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
21

Does this really help?

26–27

Are you talking about seqTy or result? Not sure if the comment moved with the update.

Add tick around intrinsic function name.

rovka added inline comments.Nov 25 2021, 5:39 AM
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
62

Add tick around instrinsic function name in Reduction.h

awarzynski added inline comments.Nov 25 2021, 6:20 AM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
8
22

Thank you all for clarifying. This is starting to make sense. I still believe that the comments for these hooks should be improved and clarify the underlying logic. Please see my suggestion at the top of this file. WDYT?

flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
21

Yes. _FortranAll is somewhat self-explanatory. But 2 is a magic number without the comment.

26–27

Both are re-defined multiple times. So is undef, dim and a few other. I'd keep only keep the bare minimum in every test that's unique to a particular case being tested. Reduced code-duplication, makes this file shorter.

clementval added inline comments.Nov 25 2021, 6:54 AM
flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
21

Fells like more work for not so much

26–27

Values have to be created right here otherwise the test does not make sense. Type can be moved to the SetUp.

awarzynski added inline comments.Nov 25 2021, 7:20 AM
flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
26–27

Values have to be created right here otherwise the test does not make sense.

Why not? Do you think that this way it will be clearer or is there a technical reason?

clementval added inline comments.Nov 25 2021, 7:31 AM
flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
26–27

No technical reason but I think it's clearer to create the operations used in the call right here.

awarzynski added inline comments.Nov 25 2021, 8:01 AM
flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
26–27

👍🏻

awarzynski added inline comments.Nov 26 2021, 3:24 AM
flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
21

Not that much work:

Add `/*fctName=*/

awk '($1 ~ /checkCallOpFromResultBox/) && !($2 ~ /fctNam/) {printf("%s /*fctName=*/%s %s\n", $1, $2, $3)}' flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp

Add `/*nbArgs=*/

awk '($1 ~ /checkCallOpFromResultBox/) && !($3 ~ /nbArgs/) {printf("%s %s /*nbArgs=*/%s\n", $1, $2, $3)}' flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp

This is not a blocker for me.

clementval added inline comments.Nov 26 2021, 4:11 AM
flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
21

Editing is quite easy as you mentioned. But updating all the patches is time consuming. I personally don't see the value of adding this. If someone else lean on your side I'll do the update.

rovka added a comment.Nov 26 2021, 4:46 AM

Just some nits, otherwise LGTM (but please wait for Andrzej and Kiran to have another look too).

flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22
29
flang/lib/Optimizer/Builder/Runtime/Reduction.cpp
455
465
554

What happens if one of these is actually used before it's implemented in the runtime? Will that be a linker error? Or is it caught sooner?

flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
225
251
280
clementval marked 13 inline comments as done.

Address review comments

flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

I'm not sure your comment represents what's happening. It describes what happens in the runtime but here we are just interested to generate the call and anything related to the runtime should be documented there.

flang/lib/Optimizer/Builder/Runtime/Reduction.cpp
554

I guess it will result in a linker error.

flang/unittests/Optimizer/Builder/Runtime/ReductionTest.cpp
225

It is actually used for Min and Max that's why it is Mxx

251

Same here used for both min and max

280

Same here used for both min and max,

awarzynski added inline comments.Nov 29 2021, 11:19 AM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

I'm not sure your comment represents what's happening.

Sure, it's just a suggestion and what I got from the conversation here. How could we improve it?

It describes what happens in the runtime

That wasn't the intention. However, as I understand, the implementation here is dictated by the implementation of the runtime. So one cannot really explain this API without discussing the runtime.

Please, feel free to discard my suggestion. However, I would really appreciate if you added some other additional documentation instead. Something that would hint the reasons behind genAllDescriptor and genAll having almost completely different signatures. That's the most visible difference here ATM.

clementval added inline comments.Nov 29 2021, 12:02 PM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

Honestly, I'm not sure I follow you here. The signature are not *completely* different. One takes a box (descriptor) to hold the result and the other is a specialized version for rank one so no box and a return value. The comments on both functions are mentioning that. How it is handle in the runtime does not belong here IMO since we are just creating a call to the runtime.

awarzynski added inline comments.Nov 30 2021, 12:15 AM
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

IMHO, the comments and naming used here are clear to somebody already familiar with this API and the wider context of how runtime calls are generated, but not so much to somebody new to this. Personally, I was missing that context when reading this file for the first time and the comments from @rovka and @kiranchandramohan really helped clarifying that. Thank you! Preserving their comments as extra documentation in this file felt like a natural and a low-cost thing to do that would be beneficial to the future Flang developers.

This is not a blocker for me.

Add comment explaining descriptor resultBox

clementval marked 7 inline comments as done.Nov 30 2021, 1:34 AM
clementval added inline comments.
flang/include/flang/Optimizer/Builder/Runtime/Reduction.h
22

Added a description extracted from comments

awarzynski accepted this revision.Nov 30 2021, 6:28 AM

LGTM, thanks for all the updates! I've left a couple of comments, but these are non-blockers.

flang/lib/Optimizer/Builder/Runtime/Reduction.cpp
723

What's the magic 0 here?

872

[nit] Not used (apart from the following line), so may just delete (it's not like ty suggests what this type is for)

This revision is now accepted and ready to land.Nov 30 2021, 6:28 AM
clementval marked an inline comment as done.Nov 30 2021, 6:45 AM

LGTM, thanks for all the updates! I've left a couple of comments, but these are non-blockers.

Thanks for the review. I'll address the two comments before landing it.

This revision was automatically updated to reflect the committed changes.
clementval marked 2 inline comments as done.

Addressed comment before landed. Mark as done.

clementval reopened this revision.Nov 30 2021, 11:57 AM
clementval marked an inline comment as not done.
This revision is now accepted and ready to land.Nov 30 2021, 11:57 AM

Add missing macro

This revision was automatically updated to reflect the committed changes.