This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add LLVMConversionTarget as a customization point. NFC.
ClosedPublic

Authored by timshen on Feb 25 2020, 2:03 PM.

Diff Detail

Event Timeline

timshen created this revision.Feb 25 2020, 2:03 PM

Can you provide a description for this patch which would motivate this please?

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
372

It isn't clear what this means, what is a "Customization class" and what can we customized?

Same as @mehdi_amini, I'd like to understand why is this necessary.

timshen updated this revision to Diff 246866.Feb 26 2020, 6:17 PM

Update comments.

Can you provide a description for this patch which would motivate this please?

Updated the comments. It's clearer to refer to D75141 and see that an action is added as illegal / dynamically legal. Without this centeralized place, we have to change all the users who write addLegalDialect<LLVM::LLVMDialect> to add the illegal action in D75141. This real intent is hard to put in the class comments, as it might get stale.

Same as @mehdi_amini, I'd like to understand why is this necessary.

See above.

timshen updated this revision to Diff 246875.Feb 26 2020, 8:27 PM

Update comments.

This real intent is hard to put in the class comments, as it might get stale.

Commit description would be the right place. https://mlir.llvm.org/getting_started/Contributing/#commit-messages -> "Prefer describing why the change is implemented rather than what it does."

ftynse accepted this revision.Feb 28 2020, 8:29 AM
This revision is now accepted and ready to land.Feb 28 2020, 8:29 AM
mehdi_amini accepted this revision.Feb 28 2020, 10:15 AM

LGTM, but please provide a complete description in the commit message when landing.

This revision was automatically updated to reflect the committed changes.