Page MenuHomePhabricator

[llvm-reduce] add ReduceAttribute delta pass
ClosedPublic

Authored by nickdesaulniers on Feb 2 2020, 12:44 PM.

Details

Summary

The output from llvm-reduce still has significantly more attributes than
bugpoint does. Teach llvm-reduce to remove attributes.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2020, 12:44 PM

Thanks for this! I'm always happy to see reduction tooling gain new powers :)

I've only touched these files in passing, so I'm probably not the best person to LGTM stamp. Just a few tiny nits.

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp
2

nit: ReduceAttributes.cpp

33

nit: looks like const std::vector<T> & would match the std::function signature in runDeltaPass, and save us a copy?

35

nit: indices are generally unsigned

49

nit: int, since this function is returning an int?

llvm/tools/llvm-reduce/deltas/ReduceAttributes.h
2

nit: ReduceAttributes.h

nickdesaulniers marked an inline comment as done.Feb 2 2020, 3:12 PM
nickdesaulniers added inline comments.
llvm/test/Reduce/Inputs/remove-attributes.py
6

yikes! indentation

nickdesaulniers marked 3 inline comments as done.
  • fix indentation, signedness, block comments
nickdesaulniers marked 2 inline comments as done.Feb 3 2020, 6:54 AM
nickdesaulniers added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp
35

The comparison below against ChunksToKeep[ChunkIndex].end would be against an int. If I change AttributesIndex to unsigned, I get the following warning:

../tools/llvm-reduce/deltas/ReduceAttributes.cpp:41:26: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
      if (AttributeIndex == ChunksToKeep[ChunkIndex].end)
          ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

since Chunk::end is an int. (Though I don't think it makes sense for chunks to ever be negative. I could change the Chunk interface if it's not a yak shave, if you'd like?)

george.burgess.iv marked an inline comment as done.Feb 3 2020, 3:38 PM

LGTM after this last nit is addressed. Like said, I plan to leave the final stamp to someone with more context. Thanks again!

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp
2

.cpp :)

35

Ick. Yeah, if you want to fix the API, feel free, but I'm happy with this as-is until that happens.

dblaikie accepted this revision.Feb 3 2020, 3:39 PM

Looks reasonable to me.

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp
53

Maybe include a matching TODO in extractAttributes?

This revision is now accepted and ready to land.Feb 3 2020, 3:39 PM
nickdesaulniers marked 2 inline comments as done.
  • update final nits
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
llvm/test/Reduce/remove-attributes.ll
3

rm -rf frightens me.

rm -f is sufficient.

Mac is failing.. http://45.33.8.238/mac/7303/step_11.txt

5

FileCheck --input-file= or FileCheck <

nickdesaulniers marked 2 inline comments as done.Feb 9 2020, 1:51 AM
nickdesaulniers added inline comments.
llvm/test/Reduce/remove-attributes.ll
3

Hmmm...I'm not sure why I cannot reproduce.

5

No other tests in this dir do that. It's required?