This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Requiring emitCInterface parameter to be explicit
ClosedPublic

Authored by wrengr on Dec 2 2021, 4:19 PM.

Details

Summary

Depends On D115004

Cleans up code legibility by requiring the emitCInterface parameter to be explicit at all call-sites, and defining boolean aliases for that parameter.

Diff Detail

Event Timeline

wrengr created this revision.Dec 2 2021, 4:19 PM
wrengr requested review of this revision.Dec 2 2021, 4:19 PM
rriddle requested changes to this revision.Dec 2 2021, 4:22 PM

Please include justification for patches in the description.

This revision now requires changes to proceed.Dec 2 2021, 4:22 PM
wrengr edited the summary of this revision. (Show Details)Dec 2 2021, 5:41 PM
mehdi_amini added inline comments.Dec 2 2021, 5:51 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
771
771

(same below)

wrengr updated this revision to Diff 391518.Dec 2 2021, 6:01 PM
wrengr marked 2 inline comments as done.

addressing comments

The default was chosen since emitCInterface is the exception when generating MLIR functions, even if the majority of our here require it.
So, let's not use defaults then, and make it explicit everywhere. As Mehdi also commented, and as I asked in the other revision
please keep /*emitCInterface=*/everywhere

(or alternatively create a CINTERFACE_ON/CINTERFACE/OFF alias for true/false, but that may be overkill here)

wrengr added a comment.Dec 3 2021, 3:25 PM

The default was chosen since emitCInterface is the exception when generating MLIR functions, even if the majority of our here require it.

I was thinking that might be the case, but wasn't certain. This differential just came about from golfing with the code (alongside D115004, D115008, D115010, and D115012, before I split them up into manageable chunks). So I'm not super attached to it, if you and River are opposed to the change.

So, let's not use defaults then, and make it explicit everywhere. As Mehdi also commented, and as I asked in the other revision
please keep /*emitCInterface=*/everywhere

(or alternatively create a CINTERFACE_ON/CINTERFACE/OFF alias for true/false, but that may be overkill here)

So we should keep the /*emitCInterface=*/ even if there's no default? So then I'm guessing the reason for the annotation is because of the bool type rather than because of the optionality (as I'd been thinking)? Is there a style guide anywhere that covers these sorts of things? (since I don't recall seeing it in the llvm nor google guides)

wrengr updated this revision to Diff 391769.Dec 3 2021, 4:05 PM

Adding aliases for bool, and removing the default for emitCInterface.

wrengr retitled this revision from [mlir][sparse] Changing the default for emitCInterface to [mlir][sparse] Requiring emitCInterface parameter to be explicit.Dec 3 2021, 4:09 PM
wrengr edited the summary of this revision. (Show Details)

The default was chosen since emitCInterface is the exception when generating MLIR functions, even if the majority of our here require it.
So, let's not use defaults then, and make it explicit everywhere. As Mehdi also commented, and as I asked in the other revision
please keep /*emitCInterface=*/everywhere

(or alternatively create a CINTERFACE_ON/CINTERFACE/OFF alias for true/false, but that may be overkill here)

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
155

For a moment, I was afraid this goes against our style, but browsing around, I see all sort of conventions for enum constants, so if lint does not complain, I guess it is okay.

Still, move enum declaration to top please, before any methods.

wrengr marked an inline comment as done.Dec 3 2021, 5:34 PM
wrengr added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
155

The linter is cool with it.

If you'd rather I use some other approach (e.g., different names, enum representation, #define in lieu of enum, etc), just let me know. I just went with the first thing that came to mind and the names you suggested :)

wrengr marked an inline comment as done.Dec 3 2021, 5:35 PM
wrengr updated this revision to Diff 391792.Dec 3 2021, 5:37 PM

Moving the enum definition to the top

aartbik accepted this revision.Dec 6 2021, 12:42 PM

New code reads nicely. Thanks!

rriddle accepted this revision.Dec 6 2021, 2:23 PM
rriddle added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
36

nit: Seems safer to use a proper enum class (and more inline with coding style)

769

Why pull out all of these adaptor.getOperands()?

This revision is now accepted and ready to land.Dec 6 2021, 2:23 PM
wrengr updated this revision to Diff 392242.Dec 6 2021, 5:29 PM
wrengr marked 2 inline comments as done.

changing enum to enum class

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
769

To avoid linewrapping is all. Same reason for doing auto call = createFuncCall(...); return call.getResult(0) all over. Though with the longer names of the enum class variation, a bunch of stuff is going to have to linewrap anyways. Then again, re-running it just now, it looks like some change affected the linter's baroque heuristics for how to linewrap things, so the results aren't as ugly as last time I tried leaving the adaptor.getOperands() inline