[Flang] flang-omp-report replace std::vector's with llvm::SmallVector

[josh.mottley.arm]

Summary

This patch replaces all uses of std::vector with llvm::SmallVector in the flang-omp-report plugin.
This is a one of several patches focusing on switching containers from STL to LLVM's ADT library.

[DavidTruby]

LGTM, wait for another review though

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

101

just putting this here as a note (this should be done in a separate patch):
This std::map should probably be changed to an llvm::DenseMap or similar

[Leporacanthicus]

LGTM.

[clementval]

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

101

You can use std::unordered_map here instead.

[DavidTruby]

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

101

Use of std::unordered_map is highly discouraged in llvm because it has portability issues across implementations. See https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options for more details

[clementval]

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

101

Unordered_set is discourage but not unordered_map. It is used widely in the code base

[awarzynski]

@DavidTruby & @clementval , many thanks for your feedback! Just as an FYI, I suggested Josh to have one patch per ADT container. As a warm-up :) @josh.mottley.arm , I suggest incorporating feedback from David in Valentin in your next patch.

[DavidTruby]

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

101

I'm not sure why unordered_set would be discouraged but not unordered_map, they're implemented the same way so would surely exhibit the same issues? The section of the manual I linked to specifically says to avoid that "hash map like" containers and then (I think erroneously) calls out unordered_set when I presume it means unordered_map. Perhaps that needs a fix in the manual?

Regardless with a quick grep through llvm I don't see many uses of unordered_map outside of libcxx (which of course defines it), especially compared to uses of DenseMap.

[clementval]

There are usage of unordered map elsewhere in the code than libcxx but if you want to go with DenseMap do whatever. It is just more complicated to put in place.

[clementval]

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

101

You probably read between the line because the section you linked does not says avoid hash map like container anywhere. It just mentions that the STL provides such containers.

The STL provides several other options, such as std::multimap and the various “hash_map” like containers (whether from C++ TR1 or from the SGI library). We never use hash_set and unordered_set because they are generally very expensive (each insertion requires a malloc) and very non-portable.

[DavidTruby]

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

101

I'm actually fairly sure this is a mistake in the guide, because this section is talking exclusively about maps and then starts talking about sets (which are covered in a separate section) for no apparent reason! That ambiguity should probably be taken up in a separate review.

Regardless elsewhere we have the following :

When both C++ and the LLVM support libraries provide similar functionality, and there isn’t a specific reason to favor the C++ implementation, it is generally preferable to use the LLVM library. For example, llvm::DenseMap should almost always be used instead of std::map or std::unordered_map, and llvm::SmallVector should usually be used instead of std::vector.

Which seems to pretty clearly discourage either map or unordered_map in favour of DenseMap

[josh.mottley.arm]

@DavidTruby & @clementval Just to let you know, got the map and string patches up for review now :)

https://reviews.llvm.org/D111977

https://reviews.llvm.org/D111980

flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h

 //===-- examples/flang-omp-report-plugin/flang-omp-report-visitor.h -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef FORTRAN_FLANG_OMP_REPORT_VISITOR_H
 #define FORTRAN_FLANG_OMP_REPORT_VISITOR_H
 
 #include "flang/Parser/parse-tree-visitor.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Parser/parsing.h"
 
+#include "llvm/ADT/SmallVector.h"
+
 #include <deque>
 #include <map>
 #include <string>
-#include <vector>
 
 namespace Fortran {
 namespace parser {
 struct ClauseInfo {
   std::string clause;
   std::string clauseDetails;
   ClauseInfo() {}
   ClauseInfo(const std::string &c, const std::string &cd)
       : clause{c}, clauseDetails{cd} {}
   ClauseInfo(const std::pair<std::string, std::string> &p)
       : clause{std::get<0>(p)}, clauseDetails{std::get<1>(p)} {}
 };
 bool operator<(const ClauseInfo &a, const ClauseInfo &b);
 bool operator==(const ClauseInfo &a, const ClauseInfo &b);
 bool operator!=(const ClauseInfo &a, const ClauseInfo &b);
 
 struct LogRecord {
   std::string file;
   int line;
   std::string construct;
-  std::vector<ClauseInfo> clauses;
+  llvm::SmallVector<ClauseInfo> clauses;
 };
 bool operator==(const LogRecord &a, const LogRecord &b);
 bool operator!=(const LogRecord &a, const LogRecord &b);
 
 using OmpWrapperType =
     std::variant<const OpenMPConstruct *, const OpenMPDeclarativeConstruct *>;
 
 struct OpenMPCounterVisitor {
struct OpenMPCounterVisitor {
   // pointers to this datastructure's entries. Hence a
   // vector cannot be used since pointers are invalidated
   // on resize. Next best option seems to be deque. Also a
   // list cannot be used since YAML gen requires a
   // datastructure which can be accessed through indices.
   std::deque<LogRecord> constructClauses;
 
   LogRecord *curLoopLogRecord{nullptr};
-  std::vector<LogRecord *> loopLogRecordStack;
-  std::vector<OmpWrapperType *> ompWrapperStack;
-  std::map<OmpWrapperType *, std::vector<ClauseInfo>> clauseStrings;
+  llvm::SmallVector<LogRecord *> loopLogRecordStack;
+  llvm::SmallVector<OmpWrapperType *> ompWrapperStack;
+  std::map<OmpWrapperType *, llvm::SmallVector<ClauseInfo>> clauseStrings;
   Parsing *parsing{nullptr};
 };
 } // namespace parser
 } // namespace Fortran
 
 #endif /* FORTRAN_FLANG_OMP_REPORT_VISITOR_H */