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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

I think it would be good in the description+commit message to reference https://reviews.llvm.org/D73853 (and that it was reverted).

Does llvm-reduce still have "interestingness tests" in `llvm/test/Reduce/Inputs/`? If so, shouldn't this change add such a test?

Thanks for the patch!



================
Comment at: llvm/tools/llvm-reduce/DeltaManager.h:36
   reduceOperandBundesDeltaPass(Tester);
+  reduceAttributesDeltaPass(Tester);
   // TODO: Implement the remaining Delta Passes
----------------
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.


================
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);
----------------
does `zip` actually simplify this sequence?  Looks kind of complicated.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:177
+    I.first->setAttributes(convertAttributeRefVecToAttributeList(C, I.second));
+  });
+}
----------------
I wish all these members of `AttributeRemapper` were `private` and these three loops maybe hidden in a method of `AttributeRemapper`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83351