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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/tools/llvm-reduce/DeltaManager.h:36
   reduceOperandBundesDeltaPass(Tester);
+  reduceAttributesDeltaPass(Tester);
   // TODO: Implement the remaining Delta Passes
----------------
nickdesaulniers wrote:
> arsenm wrote:
> > Doing this last is an improvement over bugpoint's attempt to do this first.
> > 
> > I don't think removing attributes is actually a great reduction strategy. For most of the hard to reduce testcases I debug, removing attributes is entirely pointless (and adding them is more helpful). I think this needs a flag to disable it.
> Counterpoint, I find removing attributes very helpful in reducing the amount of noise in reduced test cases and have had bugs when I needed to figure out which attribute was the source of differences in codegen.
> 
> I don't mind a flag (I don't think it's necessary, but doesn't hurt); but I'd prefer it to be default on so you can opt-out if you don't want to reduce attributes.
We currently have no such options.
How about we deal with that afterwards, by just consistently adding one for each?


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


================
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);
----------------
dblaikie wrote:
> 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.
After some refactoring...


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:155
+  });
+  sort(Res, [](const std::pair<unsigned, AttributeSet> &LHS,
+               const std::pair<unsigned, AttributeSet> &RHS) {
----------------
MaskRay wrote:
> If it is non-deterministic, use `stable_sort`
It is.


================
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));
----------------
dblaikie wrote:
> why std::for_each rather than a range-based for loop?
why range-based for loop rather than a std::for_each?

These cases i'll prefer to keep as for_each, because it really doesn't matter in which order we iterate here,
while in range-based for loop is more for traditional direct forward iteration.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:177
+    I.first->setAttributes(convertAttributeRefVecToAttributeList(C, I.second));
+  });
+}
----------------
nickdesaulniers wrote:
> I wish all these members of `AttributeRemapper` were `private` and these three loops maybe hidden in a method of `AttributeRemapper`.
The delta pass consists of three stages:
1. just counting all the features
2. enumerating each feature (in a stable order) and recording whether or not it is to be kept
3. actually applying the rewrite from the previous step

Right now, i think each step is neatly separated into `AttributeCounter`,
`AttributeRemapper` and `extractAttributesFromModule`.
I think, sinking implementation detail of the last step into middle step will convolute things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83351