It's a bit odd to see the loop ordering of a *lowering conversion* change because the output improves performance with a specific backend/compiler and for specific reasons. Moreover, that way, this would keep changing/evolving and be sensitive to common downstream paths and sensitive to LLVM's opt pipeline. In the absence of any target / scheduling info, the order to choose is typically expected to be just the most intuitive / canonical and shouldn't keep changing. You need an optimization mechanism if you need a better one.

Thanks!

@ftynse comments from the previous round sound good to me too. Looks good to commit from my side once those are addressed.

Looks good!

Looks good, thanks!

Please add a test case that exercises this. Thanks. It's not fully clear one could just ignore all uses that aren't CallOps. Please also add a comment.

constructTiledSetHyper... appears to have been moved but not mentioned in the commit summary as an NFC move. Has it been updated in some way?

Rebase.

Trigger harbormaster build check.

These build failures appear unrelated.

In D86071#2273026, @jdoerfert wrote:

Thanks for fixing the issues.

Suggestions for tweaking the information printed welcome. This change does provide more information than just "<block argument>" but still not enough (as we aren't identifying its Block in any way).

I think we definitely need a test for the builder API here to make sure it's working as intended. Please see the other builder API tests for reference in test/EDSC/builder-api-test.cpp.

In D86071#2265876, @ftynse wrote:

I guess for the general workshare loop design issues we can have an RFC in discourse. But this patch can go ahead.

I haven't seen answers to the questions about lowering to LLVM IR + OpenMP runtime, and it sounds suboptimal to push the patch before discussing and agreeing on the actual design.

In particular, it is not clear to me how this construct will connect to loops and what the lowering flow is. Does it expect an scf.for/scf.parallel as the only nested op? Is there a plan for a separate omp.for? How long do loops persist when we go to LLVM, given that OpenMPIRBuilder does not handle loop constructs and we really want to avoid converting loops to CFG during MLIR->LLVM IR translation.

Great to see this. Some minor comments.

I find the commit summary and the title a bit confusing. Isn't this extending standard dialect cast ops to work with vectors of those types? It's not that vector types are being cast to int/fp.

Please mark the commit 'NFC' (in the title). Please also include the previous commit title in the commit summary and a part of it in the current title.

I chatted with Nicolas about this earlier today, and he was okay with this idea.

Thanks for contributing this! Please state at the end of the commit summary as to which methods were just moved as a result of this revision (since there appear to be methods that were purely moved to allow reuse with constant tile size API).

Thanks for adding this option.

@dfki-ehna Thanks for fixing the posted bug. Please do rely on @herhut or @pifon2a's review - they are I assume more familiar with prior code.

Nit: commit summary: to -> due to / during the

Thanks very much for introducing this. Some initial superficial comments.

Great to see the region branch op interface being used for this. Some minor comments.

Thanks!

I'd reword the commit summary a bit to replace "Introduce" - instead, something like "Make use of ..."

Looks good.

Please fix the 80 column spillovers. Although this "Returns if ..." construction is used pervasively and isn't actually incorrect, it can be a bit confusing. "if this ..." was being used as a noun for the bool value.

I've committed this after taking care of the typo, a whitespace error, and typos in the commit title/summary.

In D86236#2238898, @AlexEichenberger wrote:

@bondhugula I don't believe I have commit privilege yet. Please commit for me, if you don't mind.

@AlexEichenberger Let me know if you are able to commit or if you'd like me to commit this for you.

Thanks!

Some more minor comments - mostly on documentation and comments. Can you please fix?

In D86392#2232403, @mehdi_amini wrote:

In D86392#2232401, @bondhugula wrote:

You may also want to add an [MLIR] tag to the title to not confuse LLVM proper folks.

I think this is noise in my git log and emails: it is likely better solved with tooling.