[PATCH] D83351: [llvm-reduce] Reducing attributes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:60-61
+  void visitModule(Module &M) {
+    for_each(M.getGlobalList(),
+             [&](GlobalVariable &GV) { visitGlobalVariable(GV); });
+  }
----------------
range-based-for loop, probably?


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:107-108
+
+  AttributeCounter() {}
+
+  void visitModule(Module &M) {
----------------
drop this since it's implicit/default?


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:146-148
+  for (const auto &I : zip(Res, AttributeSets)) {
+    std::pair<unsigned, AttributeSet> &NewSet = std::get<0>(I);
+    const AttrPtrIdxVecVecTy &V = std::get<1>(I);
----------------
nickdesaulniers wrote:
> does `zip` actually simplify this sequence?  Looks kind of complicated.
+1 to this.
```
std::vector<std::pair<unsigned, AttributeSet>> Res;
Res.reserve(AttributeSets.size());
for (const auto &V : AttributeSets)
  Res.push_back({V.first, convertAttributeRefToAttributeSet(C, V.second)});
```

Seems simpler.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:152-154
+  erase_if(Res, [](const std::pair<unsigned, AttributeSet> &NewSet) {
+    return !NewSet.second.hasAttributes();
+  });
----------------
Could roll this into the previous for loop, making the push_back conditional:
```
std::vector<std::pair<unsigned, AttributeSet>> Res;
Res.reserve(AttributeSets.size());
for (const auto &V : AttributeSets) {
  AttributeSet S = convertAttributeRefToAttributeSet(C, V.second);
  if (S.hasAttributes())
    Res.push_back({V.first, S}); // maybe std::move(S)? Not sure if it's sufficiently heavy to benefit from moving
}
```


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:169
+  LLVMContext &C = Program->getContext();
+  for_each(R.GlobalVariablesToRefine, [&C](const auto &I) {
+    I.first->setAttributes(convertAttributeRefToAttributeSet(C, I.second));
----------------
why std::for_each rather than a range-based for loop?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83351/new/

https://reviews.llvm.org/D83351