no
User Details
- User Since
- Nov 17 2016, 12:59 PM (293 w, 5 d)
Yesterday
Fri, Jun 24
LGTM
Mon, Jun 6
Ok, LGTM then
@t.p.northover What do you think about this?
I think this looks reasonable now? I basically only have one nit.
I think this seems reasonable, but if this is a problem with the verifier, why not fix it there? Or would that not work?
May 17 2022
Can you add test cases for:
May 16 2022
Do the outliner changes need to be a part of this patch? Or can you split the outliner changes out into a separate patch, and then add that separate patch as a child of this one?
May 13 2022
LGTM, thanks!
Aside from @foad's one comment, I think this looks fine.
May 12 2022
I think this seems reasonable? Looks like a compile time improvement too, maybe?
LGTM
May 6 2022
May 4 2022
May 3 2022
LGTM
May 2 2022
I think this is fine. I have a couple nits on the comments, but I won't hold up review over them.
Apr 22 2022
LGTM
Apr 21 2022
LGTM
Apr 18 2022
I think that debug info handling might be a good argument for splitting the similarity identifier into a similarity finding version + outlining version. The outliner needs to be considerate about what it does to debug info, but the similarity identifier doesn't. Someone very well may want to ask questions about debug info similarity in the future.
Apr 15 2022
LGTM
LGTM
LGTM
LGTM
@mtrofin Everything these bots does is sets up a fresh environment each time, so there ought to be nothing left over from previous runs. Everything is done in a new directory each time IIRC, so we get a clean build every time.
Apr 14 2022
This is also hitting the x86-64 macOS bots 😢
You know what, I think I wonked something up somehow on my end, because I rebuilt + reran the test suite overnight with -Oz + -O2 and it seems fine.
Apr 13 2022
I applied this patch locally and tried to run the test suite at -O2 with it with asserts enabled. It fell over building fpcmp.
I think this is fine.
Apr 12 2022
Ah. There are no C++ unit tests that can be updated without an immediate use case?
LGTM
LGTM
LGTM
Testcase?
I think this is a good cleanup. Even if we get some slightly worse codegen at -O0, it simplifies the selector + legalizer. LGTM
LGTM
Apr 11 2022
Oh, I see
Also small bug fix that prevented the correct insertion of G_ASSERT_ZEXT in the AArch64 use case.
LGTM
Apr 5 2022
Mar 30 2022
Mar 28 2022
Code looks equivalent to the fpimm32SIMDModImmType4XForm in AArch64InstrFormats.td, which looks fine to me.
Mar 25 2022
LGTM with nit on a comment
Mar 23 2022
Mar 16 2022
I think after the one nit is fixed, this is good to go.
Mar 14 2022
Also, which GitHub issue is this associated with?
LGTM
This testcase is better, but there are still lots of instructions which aren't necessary.
FWIW I think we could shave off some compile time if we could make some sort of interface to the similarity identifier that tells it "this is for outlining". That may just be a subclass or something of the similarity identifier.
Mar 9 2022
LGTM
Yeah, it looks like there's a clang attribute, but it isn't used anywhere in the LLVM test suite.
Do you think you could split this into two patches?
Mar 8 2022
IIRC I was working on something similar in the combiner but for FP stuff (D116702)
Mar 7 2022
Mar 4 2022
I'm not sure how often musttail/tailcc appears in the LLVM test suite, but this didn't introduce any asserts there, and all of the tests pass.
Just verified that this resolves the crash in the benchmark on my end.
Mar 3 2022
Do you think you could add a statistic which counts the number of tail calls emitted? And maybe some debug output that says that something will be handled as a tail call?
LGTM
LGTM
I am a little worried about the code extractor's assumptions, at least wrt noreturn functions (should the code extractor have a special case for noreturn?). But I think that should be handled in the code extractor, not here.
Mar 2 2022
Oh, good news, for the other issue, I managed to repro without the patch:
I think we can go ahead and land this.
I measured myself and found that this actually exposes *another* bug.
Mar 1 2022
What is the code size impact on the test suite?
Feb 23 2022
Yeah I was pretty sure this was NFC at first, but... I guess not! Going to revert.
Feb 21 2022
Feb 18 2022
LGTM
Feb 17 2022
Feb 16 2022
Looks straightforward to me?
Feb 8 2022
Filtering out non-integral spaces for now makes sense to me.
Feb 7 2022
I think this looks fine