Page MenuHomePhabricator

[llvm] Make Sequence reverse-iterable
ClosedPublic

Authored by gchatelet on Jun 9 2021, 1:40 AM.

Details

Summary

This is a roll forward of D102679.
This patch simplifies the implementation of Sequence and makes it compatible with llvm::reverse.
It exposes the reverse iterators through rbegin/rend which prevents a dangling reference in std::reverse_iterator::operator++().

Note: Compared to D102679, this patch introduces a asSmallVector() member function and fixes compilation issue with GCC 5.

Diff Detail

Event Timeline

gchatelet created this revision.Jun 9 2021, 1:40 AM
gchatelet requested review of this revision.Jun 9 2021, 1:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
gchatelet added inline comments.Jun 9 2021, 1:48 AM
llvm/include/llvm/ADT/Sequence.h
17–18

@Quuxplusone @mehdi_amini Introducing the asSmallVector() method add a dependency in Sequence.
I'm having a hard time evaluating the costs/benefits balance of this move.

178

@Quuxplusone I will remove the std::move in this patch (cf. D103900)

mlir/include/mlir/IR/BuiltinAttributes.td
711

The purpose of this change was to prevent mlir from depending on implementation details of llvm.

@mehdi_amini I've added typename to disambiguate between value and type for GCC5 but I have no way of checking whether that works or not (GCC5 is not available on my machine).
Do you have any suggestions on how to test it?

gchatelet updated this revision to Diff 350821.Jun 9 2021, 1:52 AM
  • Add missing header for std::ptrdiff_t
  • Remove unnecessary std::move
Quuxplusone accepted this revision.Jun 9 2021, 6:43 AM
Quuxplusone added inline comments.
llvm/include/llvm/ADT/Sequence.h
153

Nit: Just std::ptrdiff_t seems simpler for the reader.

159–164

I still believe this should be simply

explicit iota_range(ValueT Begin, ValueT End)
      : Begin(Begin), End(End) {}

with no templates. (For the same reason as lines 182-183.)
And explicit because all constructors should be explicit.

llvm/unittests/ADT/SequenceTest.cpp
42–48

/Dereferene/Dereference/

mlir/include/mlir/IR/BuiltinAttributes.td
711

Indeed the error message looked to me like the symptom of "forgetting to include the right header somewhere," because the compiler seemed to be complaining that it didn't know what ptrdiff_t meant. Adding typename here also LGTM.
Nit: const_iterator could be just iterator, right? There's no difference between const-iterating and regular-iterating an llvm::seq, so the difference between them is nonexistent and we might as well use the shorter simpler name.

This revision is now accepted and ready to land.Jun 9 2021, 6:43 AM
mehdi_amini added inline comments.Jun 9 2021, 2:41 PM
mlir/include/mlir/IR/BuiltinAttributes.td
711

I usually use a docker container to test this, but feel free to land it, we can always revert it if it breaks a bot again.

gchatelet updated this revision to Diff 351127.Jun 10 2021, 4:12 AM
gchatelet marked 5 inline comments as done.
  • Rebase and address comments
This revision was landed with ongoing or failed builds.Jun 10 2021, 4:15 AM
This revision was automatically updated to reflect the committed changes.