- User Since
- Jul 7 2012, 2:54 PM (363 w, 3 d)
Personally, I'd suggest the name llvm-reduce for the tool.
Fri, Jun 21
FWIW, I think we can wait for a bug to be filed or report come in with some functional test case before we change any behavior here.
Thu, Jun 20
Eh, this seems close enough now. I'd like a better approach for the x86 builtins, but no idea what it will end up being.
See inline comment, but I think we should just drop the testing of the function attribute bit here rather than adjusting the pipeline.
(Will likely need more eyes than just mine -- RPATH is mostly a mystery to me...)
Wed, Jun 19
just a minor comment on one of these...
LGTM, again, really nice. Tiny tweak below.
Tue, Jun 18
FWIW, this LGTM. We may want to tweak the behavior around flattening, but that can happen as a follow-up, and this seems to get us to a better state.
OMG, I'm so sorry, I had no idea that the tests would explode like that... Yeah, I don't think that's useful....
Wed, Jun 12
LGTM. Bit annoying that we need to do this at O0, but fine...
Code change LGTM, but again, let's update the test to check both ways.
I understand the change to explicitly say -O2. I also understand the change to add an explicit -fno-experimental-new-pass-manager to a RUN line when we have another RUN line that explicitly uses -fexperiemntal-new-pass-manager.
Let's update the test to explicitly run w/ both PMs to make sure this keeps working. LGTM with that change.
Code change LGTM. Can you update at least one of the tests to explicitly run both PMs so that we'll notice if this breaks in some weird way? Feel free to submit with that change.
This really confused me. We shouldn't be seeing this kind of difference in the new PM.
This was a lot easier for me to understand too, thanks. Somewhat minor code changes below.
Mon, Jun 10
I think this is somewhat the wrong approach.
I think this ultimately needs to be split up into smaller patches. A bunch of these things can be landed independently. Here is my first cut at things to split out, each one into its own patch.
I would just change this to have the module pass loop over the functions -- that seems like it'll be much cleaner.
Sat, Jun 8
Any chance you can add a test that shows one of the problems where verifying MemorySSA fails because we get a "preserved" copy after a loop pass that doesn't actually preserve it?
Thu, Jun 6
Joerg points out in IRC that we need control flow pruning to be symmetric w/ true and false.
(For posterity, this also should be in the implementation of SimplifyInstruction itself, not in the pass. The pass only ever calls that.)
I think this isn't quite right (if I've understood what it is doing correctly).
May 22 2019
Sorry I've been a bit slow to respond here...
May 18 2019
May 17 2019
Ok, I'm fine with this going in temporarily while we try to flip this flag acros sthe board (or figure out a heuristic that lets us get the best of both worlds). So, LGTM, but yeah, let's try to remove this too.
Eh, I'd like long term to have a way of testing this field in LLVM, but I think this is fine for now to unblock the Clang changes and other changes.
We should get folks to figure out why this regresses them, but making the two PMs behave the same seems fine for now.
One edge case left I think, but otherwise this LGTM. Happy for you to submit with the suggested change and a test case.
May 15 2019
I think you mentioned that there is already a test case that checks a global initialized with blockaddress? If I've mis-remembered, we should definitely add one and check that it *doesn't* inline (today). We can update it if/when we add support. If it is possible to add one that directly works w/ callbr, that'd be great.
LGTM as long as Eli is ok with the testing arriving after the fact.
May 10 2019
SLH bits LGTM.
May 7 2019
(Sorry for still more comments)
The file name of the test seems odd? How about vectorize-loops.c? I'd also make it a C test and put it in test/CodeGen instead of a C++ test.
May 6 2019
(To be explicit, this LGTM, just letting George mark it as accepted when he's happy too.)
This seems... *really* unlikely to be an important tuning flag for library clients. It seems likely added to allow some limited experimentation. Maybe we can just not add it to the new PM?
Same comment as on other Clang patch -- let's update an IR generation test to reflect this?
Hmm, how should we test that this works?
Can you update some Clang IR generation test that uses these flags to run w/ the new PM? It should fail without this and pass with this.
Eric was hoping for some API improvements here as we add more tuning options, but I don't think these need to be sequenced.
May 4 2019
One question about a test change and a minor nit pick on comments...
I'm trusting George w/ the MemorySSAUpdater review.
Apr 29 2019
LGTM, very nice! Thanks for all the work tracking down this subtle case!
LGTM, maybe add a comment here to archive some of the reasoning? (IE, that when unrolling is disabled we disable both literal unrolling but also interleaving for historical reasons)
Apr 25 2019
You might also update one of the instr prof tests to have two RUN lines, one for each pass manager. Feel free to do that (or not) and submit.
Apr 24 2019
I like the patch, LGTM.
Apr 23 2019
How does this impact use of clang -fno-unroll-loops and clang -fno-loop-vectorize? I'm betting today, -fno-unroll-loops controls the DisableUnrollLoops value, and so people who disable both unrolling and vectorization will be surprised when interleaving now happens.
Oh, you can add a test for this by using the new PM and an invalidate<aa> pass or invalidate<domtree>.
Apr 22 2019
Apr 19 2019
This looks right to me. Some suggestions on comment wording only.
Apr 18 2019
Mostly questions around the AA manager preservation, see inline.
LGTM with comments below addressed. All pretty minor.
Apr 17 2019
It looks like this is a completely new approach to attribute inference compared to the post-order function atters pass?
Generally really nice. Some suggestions on cleaning up comments and such in the tests.
FWIW, I'm not really a fan of this.
Apr 8 2019
I think the approach here looks really good. Comments below.
Apr 4 2019
Mar 29 2019
While I think we should figure out how to test this, we shouldn't wait to land what seems like an important correctness fix on figuring out how to test. So mostly looking for a particular change below.
Mar 27 2019
Fix up the CHECK lines in my new test case.