Page MenuHomePhabricator

[mlir] Replace some std::tie uses with structured bindings
AbandonedPublic

Authored by jloser on Aug 10 2022, 8:20 PM.

Details

Summary

std::tie for local variables that are non-modifying is no longer needed in
C++17. Replace these instances with structured bindings.

Diff Detail

Event Timeline

jloser created this revision.Aug 10 2022, 8:20 PM
Herald added a project: Restricted Project. · View Herald Transcript
jloser requested review of this revision.Aug 10 2022, 8:20 PM
rriddle accepted this revision.Aug 10 2022, 8:58 PM
rriddle added inline comments.
mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp
426–429

We could also do this, right? Not sure how that formats though.

This revision is now accepted and ready to land.Aug 10 2022, 8:58 PM
mehdi_amini requested changes to this revision.Aug 11 2022, 4:28 AM

I have some readability concerns, I think this should be solved by an RFC on the style guide.

mlir/examples/toy/Ch7/mlir/MLIRGen.cpp
459 ↗(On Diff #451711)

I see this kind of change as a regression in terms of readability: the types are no longer explicit and we're forced into auto instead.

This revision now requires changes to proceed.Aug 11 2022, 4:28 AM

Seems like the standard hasn't moved on https://isocpp.org/files/papers/p0480r1.html :(

I think going from

if (auto it : llvm::zip(a, b)) 
  thing(std::get<0>(it), std::get<1>(it));

to

if (auto [x, y] : llvm::zip(a, b)) 
  thing(x, y);

This at least is unambiguously better I feel.

I think going from

if (auto it : llvm::zip(a, b)) 
  thing(std::get<0>(it), std::get<1>(it));

to

if (auto [x, y] : llvm::zip(a, b)) 
  thing(x, y);

This at least is unambiguously better I feel.

Right, this is a different case as there was already auto. This patch is about replacing std::tie as far as I can tell.

jloser added a comment.EditedAug 11 2022, 1:56 PM

I think going from

if (auto it : llvm::zip(a, b)) 
  thing(std::get<0>(it), std::get<1>(it));

to

if (auto [x, y] : llvm::zip(a, b)) 
  thing(x, y);

This at least is unambiguously better I feel.

Right, this is a different case as there was already auto. This patch is about replacing std::tie as far as I can tell.

Yes, this patch is targeting the few uses of std::tie in contexts that don't modify references to function local or class member variables. Most of the std::tie uses, unfortunately, fall into the category I just mentioned around references.

When poking around, I also had the same observation of the use of explicit std::get<N> in lieu of structured bindings. I agree with you that structured binding use is clearly better in those cases. I intentionally didn't couple those patterns with this patch; I'm happy to explore those in a separate patch as it improves readability IMO. How does that sound?

For example (no pun intended), in the examples, we'd replace

for (const auto nameValue :
     llvm::zip(protoArgs, entryBlock.getArguments())) {
  if (failed(declare(std::get<0>(nameValue)->getName(),
                     std::get<1>(nameValue))))
    return nullptr;
}

with

for (const auto [name, value] :
     llvm::zip(protoArgs, entryBlock.getArguments())) {
  if (failed(declare(name->getName(), value)))
    return nullptr;
}
mlir/examples/toy/Ch7/mlir/MLIRGen.cpp
459 ↗(On Diff #451711)

I'll revert this part of the patch. I agree this isn't clearly better and we should strive for readability and understanding of types with these examples.

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp
426–429

In general, yes, that's a safe change. I don't think we should necessarily do that here though. Later on in this function, we do Value originalBound = std::get<3>(config), so we'd need to copy the upperBound value for use later on if we remove the config variable, but the local variable definition would have to be a bit more removed from its use which is not ideal (we'd need to modify it before potentially changing the upperBound in one of the branches).

I think I'll keep the code as-is unless you feel strongly.

jloser updated this revision to Diff 451975.Aug 11 2022, 1:59 PM

Revert structured binding use in the example

I'm less concern about the StringRef case (I guess it is obvious enough that "splitting" a StringRef returns two StringRef).

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp
431

We're losing Type qualifier here as well...

jloser abandoned this revision.Aug 14 2022, 8:52 AM

Given the concerns about losing the explicit type information for variable declarations, I'm no longer going to pursue these changes.

I'll look into the zip/std::get<N> use in a different patch and we can see what we think of those use of structured bindings. Most of those from a quick grep around already use auto so it won't be a regression in terms of type information to readers.