Page MenuHomePhabricator

[mlir] make vector to llvm conversion truly partial
ClosedPublic

Authored by ftynse on Jan 29 2021, 8:46 AM.

Details

Summary

Historically, the Vector to LLVM dialect conversion subsumed the Standard to
LLVM dialect conversion patterns. This was necessary because the conversion
infrastructure did not have sufficient support for reconciling type
conversions. This support is now available. Only keep the patterns related to
the Vector dialect in the Vector to LLVM conversion and require type casts
operations to be inserted if necessary. These casts will be removed by
following conversions if possible. Update integration tests to also run the
Standard to LLVM conversion.

There is a significant amount of test churn, which is due to (a) unnecessarily
strict tests in VectorToLLVM and (b) many patterns actually targeting Standard
dialect ops instead of LLVM dialect ops leading to tests actually exercising a
Vector->Standard->LLVM conversion. This churn is a good illustration of the
reason to make the conversion partial: now the tests only check the code in the
Vector to LLVM conversion and will not be randomly broken by changes in
Standard to LLVM conversion.

Arguably, it may be possible to extract Vector to Standard patterns into a
separate pass, but given the ongoing splitting of the Standard dialect, such
pass will be short-lived and will require further refactoring.

Depends On D95626

Diff Detail

Event Timeline

ftynse created this revision.Jan 29 2021, 8:46 AM
ftynse requested review of this revision.Jan 29 2021, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2021, 8:46 AM
aartbik accepted this revision.Jan 29 2021, 11:20 AM

Oh this is nice! I always felt like vector dialect lowered too much, making the tests more LLVM-centric than they should be, but this resolves it nicely.
Please wait for Nicolas to approve too, but LGTM

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVMPass.cpp
73–74

don't you want to remove this for real now instead of leaving it commented out ;-)

88

I have the weird request to remove all empty lines in this new code, just because all the architectural specific additions were in one straight block, but I can see how that request is very subjective, so feel free to ignore as well....

This revision is now accepted and ready to land.Jan 29 2021, 11:20 AM

Great thanks, double benefit of killing most LLVM types :)

Thanks!

ftynse updated this revision to Diff 321351.Feb 4 2021, 2:21 AM

Address review.

This revision was landed with ongoing or failed builds.Feb 4 2021, 2:33 AM
This revision was automatically updated to reflect the committed changes.