chandlerc (Chandler Carruth)Administrator
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 7 2012, 2:54 PM (254 w, 4 d)
Roles
Administrator

Recent Activity

Today

chandlerc added a comment to D33535: [PM] Teach the PGO instrumentation pasess to run GlobalDCE before instrumenting code..

Thanks, landing this to fix the immediate issue (with comments addressed).

Wed, May 24, 11:43 PM
chandlerc committed rL303843: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch.
[PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch
Wed, May 24, 11:33 PM
chandlerc closed D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass. by committing rL303843: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch.
Wed, May 24, 11:33 PM
chandlerc added a comment to D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..

Over the past week or so, I think we have done enough playing around with incremental dominators at this point (and i believe jakub has working prototype code) that i feel confident enough that we can make it work, and so i'm not worried about this patch really.
Maybe it's code we destroy in a few months, but it doesn't seem worth holding up progress over.
It obviously could turn out i'm completely wrong (IE as we bring a design to upstream and start on a non-prototype, something becomes intractable), but we can still deal with it then.

Wed, May 24, 11:06 PM
chandlerc added a reviewer for D33535: [PM] Teach the PGO instrumentation pasess to run GlobalDCE before instrumenting code.: davidxl.

Add David to the reviewer set in case he can look sooner. I'd love to land this and unblock testing of the new PM + PGO.

Wed, May 24, 10:36 PM
chandlerc added a comment to D33535: [PM] Teach the PGO instrumentation pasess to run GlobalDCE before instrumenting code..

Forgot to mention, I also added a test to verify that GlobalDCE won't remove available_externally functions that are actually in use in the module so it won't impair subsequent optimization passes.

Wed, May 24, 10:15 PM
chandlerc created D33535: [PM] Teach the PGO instrumentation pasess to run GlobalDCE before instrumenting code..
Wed, May 24, 10:15 PM
chandlerc added inline comments to D33533: [IR] Add an iterator and range accessor for the PHI nodes of a basic block..
Wed, May 24, 8:12 PM
chandlerc committed rL303834: [LegacyPM] Make the 'addLoop' method accept a loop to add rather than.
[LegacyPM] Make the 'addLoop' method accept a loop to add rather than
Wed, May 24, 8:01 PM
chandlerc closed D33528: [LegacyPM] Make the 'addLoop' method accept a loop to add rather than having it internally allocate the loop. by committing rL303834: [LegacyPM] Make the 'addLoop' method accept a loop to add rather than.
Wed, May 24, 8:01 PM
chandlerc added a comment to D33528: [LegacyPM] Make the 'addLoop' method accept a loop to add rather than having it internally allocate the loop..

This is fine assuming you audited all the callers (a quick grep shows just these two).

Wed, May 24, 7:56 PM
chandlerc created D33533: [IR] Add an iterator and range accessor for the PHI nodes of a basic block..
Wed, May 24, 7:55 PM
chandlerc added a comment to D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..

FYI, I'm still waiting to hear back from you Danny...

Wed, May 24, 7:17 PM
chandlerc created D33528: [LegacyPM] Make the 'addLoop' method accept a loop to add rather than having it internally allocate the loop..
Wed, May 24, 5:39 PM

Yesterday

chandlerc added a comment to D33428: [PDB] Hash types up front when merging types instead of using StringMap.

Drive by comment...

Tue, May 23, 3:28 PM
chandlerc added a comment to D33467: Fix LLVM build errors if necent build of GCC 7 is used.

What error are you trying to fix? We use flags without .getValue() all over the place, I don't know why we would need to change that here.

Tue, May 23, 3:25 PM

Mon, May 22

chandlerc updated the diff for D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..

Fix comment even harder.

Mon, May 22, 10:17 PM
chandlerc added inline comments to D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..
Mon, May 22, 10:09 PM
chandlerc added a comment to D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..

I have an intern starting in two weeks to do incremental dominator algorithms :)

Mon, May 22, 10:04 PM
chandlerc updated the diff for D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..

Update the comments to be more accurate and re-arrange the FIXME.

Mon, May 22, 9:54 PM
chandlerc added a comment to D32819: [IR] Switch AttributeList to use an array for O(1) access.

(FWIW, given the overall plan, I'm very happy for Hans to handle the code review.)

Mon, May 22, 1:47 PM

Sun, May 21

chandlerc added a comment to D32737: [Constants][SVE] Represent the runtime length of a scalable vector.

There is a lot of discussion here that I really don't think should be on a patch review. It should be an an llvm-dev thread. See below.

From my point of view, making scalable vectors a native and core type of IR is the only way forward, because the semantics needs to be ingrained in the language to make sense. AFAICS, at the IR level, the differences between vectors and scalable ones is not that big a deal, certainly not bigger than vectors versus scalar.

This is much more up for debate. Details matter. Compilers are a set of engineering tradeoffs. You need to explain and defend your position carefully, not just state it as though it were "obviously true".

I though that was clear from my previous response to you: "I don't think anyone disagrees with that point."

I'm certainly not asking people to "trust me, I'm an engineer". We had previous discussions in the list, other people chimed in. This is not even my patch and I have nothing to do with their work... "I just work here".

I'm also certainly not asserting that I know all the answers and that this change is worry free, by any means, and that's precisely why I pinged more people to give their opinions, because I was uncomfortable with the low amount of reviews. But it was certainly not "just me".

Do you have a discussion somewhere of the overall design of the extensions you're proposing? This is presumably only a small piece of it.

There were discussions on the list, a "whole-patch approach" in Github and some previous patches. I can't find any of it (my mail client - gmail - is a mess). I'll let Graham cover that part.

Yes, but in that discussion, I specifically asked for a new, fresh RFC thread that reflected substantial changes made to the design presented in the first RFC email through the discussion. That thread, AFAICT, never happened. If I missed, it I'm happy to get a pointer to it. If not, the author of this patch should start it. Either way, that is where this discussion should take place. I think there remain a lot of important technical issues here. I would like to dig into them, but I *don't* want to do it here where I don't even have the complete design.

I would encourage everyone (Hal, Sanjoy, etc) to hold off on debating "how does X work" here and redirect that to the (either existing or eventual) llvm-dev thread with an updated overall design.

Sun, May 21, 1:26 PM
chandlerc added a comment to D32737: [Constants][SVE] Represent the runtime length of a scalable vector.

There is a lot of discussion here that I really don't think should be on a patch review. It should be an an llvm-dev thread. See below.

Sun, May 21, 1:25 PM

Sat, May 20

chandlerc added a comment to D33125: Introduce isoneof<T0, T1, ...> as an extension of isa<T>.

Another high-level comment: I don't like the name isoneof. To me, that parses to the relationship being tested is rather than isa which loses an important distinction. I'm not really sure what would be the best name though.

Sat, May 20, 4:53 PM
chandlerc added a comment to D33125: Introduce isoneof<T0, T1, ...> as an extension of isa<T>.

I'd like to request that you make this patch just introduce the tool, and maybe add a few usages. I think rewriting this much in a single patch seems like a bad idea. It'll make reverts if anything goes wrong pointlessly hard, etc.

Sat, May 20, 4:51 PM
chandlerc added a comment to D32737: [Constants][SVE] Represent the runtime length of a scalable vector.

@echristo @chandlerc @lattner @majnemer Ping.

This is a trivial change, discussed in the past, and I'm inclined to approve.

Sat, May 20, 2:08 PM

Fri, May 19

chandlerc added a comment to D33059: Create an shared include directory for gtest helper functions.

but I'm not sure if sticking our own directory inside of a directory of third party code would be weird.

I also think i would be weird. I'd like us to keep it separate.

Bear with me here. Would it be more or less weird to have a directory of the same name but in a different location?

e.g.

utils
  googlemock
    include
      gmock
  googletest
    include
      gtest
  Support
    include
      gtest
        Support

This way you could still write #include "gtest/Support/ErrorChecking.h" but we wouldn't be stepping on third party code.

Fri, May 19, 11:33 AM
chandlerc added a comment to D33059: Create an shared include directory for gtest helper functions.

Moved the folder.

Fri, May 19, 11:27 AM
chandlerc added inline comments to D33164: [Profile[ Enhance expect lowering to handle correlated branches.
Fri, May 19, 11:04 AM

Thu, May 18

chandlerc added a comment to D33341: Enable vectorizer-maximize-bandwidth by default..

To get more feedback, you might want to send an llvm-dev email with the benchmark data you have and ask others to benchmark by passing the flag there? More folks would see that email, and you can point at this patch as a place to have detailed discussion.

Thu, May 18, 3:15 PM

Wed, May 17

chandlerc accepted D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.
Wed, May 17, 9:14 PM
chandlerc added a comment to D33306: [IfConversion] Make the ifcvt-limit command line option work at the function level and remove compares with global Statistic variables.

Maybe we should switch to a DebugCounter?

Wed, May 17, 9:04 PM
chandlerc added a comment to D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.

Hello Guys,

To make a progress with landing this patch I need a feedback from you. As I see:

  1. Chandler had no issues with this patch
  2. David in his e-mail wrote that he is ok with this patch
  3. Duncan made a proposal to dd some tests with switch.
Wed, May 17, 8:05 PM
chandlerc added a comment to D33306: [IfConversion] Make the ifcvt-limit command line option work at the function level and remove compares with global Statistic variables.

So, I think this was actually working as intended.

Wed, May 17, 7:52 PM
chandlerc added inline comments to D33059: Create an shared include directory for gtest helper functions.
Wed, May 17, 5:01 PM
chandlerc added a comment to D33059: Create an shared include directory for gtest helper functions.

I definitely want this. First question is where?

Wed, May 17, 4:57 PM
chandlerc accepted D33301: [Statistics] Add a method to atomically update a statistic that contains a maximum.

Ahh, "atomic" max. I remember when I first wrote this in some other project 8 years ago now and had to figure out if it really has adequate forward progress guarantees... =] (It does.)

Wed, May 17, 4:51 PM
chandlerc added a comment to D33222: [LegacyPassManager] Remove TargetMachine constructors.

This LGTM. I'd get one from Chandler as well though.

Wed, May 17, 4:13 PM
chandlerc accepted D31261: [IR] De-virtualize ~Value to save a vptr.

LGTM with some nits! Totally awesome.

Wed, May 17, 2:30 PM
chandlerc accepted D33104: [BitVector] Implement find_[first/last]_[set/unset]_in.

LGTM with the assert changes.

Wed, May 17, 8:36 AM
chandlerc added a comment to D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently.
In D30416#756899, @wmi wrote:

Discussed with Chandler offline, and we decided to split the patch and tried to commit the store shrinking first.

Then I tried the idea of walking forward for load shrinking by using demandedbits but I run into a problem for the motivational testcase (test/CodeGen/X86/mem-access-shrink.ll). Look at the %bf.load which we want to shrink in mem-access-shrink.ll, it has multiple uses, so we want to look at all its uses and get the demanded bits for each use. However, on the Def-Use chain from %bf.load to its narrower uses, it is not only %bf.load having multiple uses, for example, %bf.set also has multiple uses, so we also need to look at all the uses of %bf.set. In theory, every node on the Def-Use Chain can have multiple uses, then at the initial portion of Def-Use Chain starting from %bf.load, we don't know whether the %bf.load can be shrinked or not from demanded bits, only when we walk pretty close to the end of the Def-Use Chain, we know whether %bf.load can be shrinked at the specific place. In other words, by walking forward, in order not to miss any shrinking opportunity, we have to walk across almost all the nodes on the Def-Uses tree before knowing where %bf.load can be shrinked.

For walking backward, most of the cases, we only have to walk from a narrower use to "%bf.load =" which is the root of the Def-Uses tree. It is like walking from some leafs to root, which should be more efficient in most cases. I agree we may see special testcase like there is a long common portion for those paths from leafs to root (for that case walking forward is better). If that happen, we can add a cap about the maximum walking distance to avoid the compile time cost from being too high. Chandler, do you think it is ok?

Wed, May 17, 12:31 AM

Tue, May 16

chandlerc requested changes to D31261: [IR] De-virtualize ~Value to save a vptr.

Have you checked Polly and Clang for changes needed there? Detailed comments below.

Tue, May 16, 7:04 PM
chandlerc requested changes to D33104: [BitVector] Implement find_[first/last]_[set/unset]_in.

Very nice generally. Comments only on one, but clearly apply to each variation.

Tue, May 16, 6:55 PM
chandlerc added inline comments to D33164: [Profile[ Enhance expect lowering to handle correlated branches.
Tue, May 16, 5:08 PM
chandlerc requested changes to D33164: [Profile[ Enhance expect lowering to handle correlated branches.
Tue, May 16, 3:06 PM
chandlerc accepted D33157: [Inliner] Do not mix callsite and callee hotness based updates..

LGTM again! =D

Tue, May 16, 2:16 PM
chandlerc requested changes to D33157: [Inliner] Do not mix callsite and callee hotness based updates..

(Just marking this as no longer ready to go so it shows back up in my queue...)

Tue, May 16, 11:02 AM
chandlerc added a comment to D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.

As an aside, it seems the logic around unreachable (with and without the patch) might pessimize code paths that lead to noreturn functions, such as posix_spawn(). Is that really what we want? Why?

Tue, May 16, 10:55 AM
chandlerc added a comment to D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.

That is not my question :)

My question is that without the patch, the probability of UR branch is actually lower (1, 0x8000....). Looks like it gets truncated somewhere? In other words, the fix can be put to where the truncation happens.

Tue, May 16, 10:07 AM
chandlerc added a comment to D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.

It is known that BFI can not handle zero weight and it will add 1 to zero weight when it sees it.

Here the unreachable Branch prob is not zero (without the change). Where does it get changed to zero?

Tue, May 16, 9:12 AM
chandlerc added a comment to D33222: [LegacyPassManager] Remove TargetMachine constructors.

Cool! This seems to me like a really nice simplification given the fact that the LPM has significant constraints on how you construct passes.

Tue, May 16, 9:06 AM
chandlerc added reviewers for D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0: davidxl, dexonsmith.

This at least seems like a very reasonable patch to me. I'd like to double check with David and Duncan as well (I've added them as reviewers). My only lingering concern would be if this will run the risk of underflow coming up more often, but that is probably not too much of an issue now that BFI does a better job of things.

Tue, May 16, 7:45 AM

Fri, May 12

chandlerc added a comment to D33097: [x86] Follow-up to r302785 - don't widen vselect conditions when the vNi1 type being split into is a legal type (like it usually is w/ AVX-512)..

Ping... Seems like this should be a simple patch?

Fri, May 12, 11:43 PM
chandlerc accepted D33157: [Inliner] Do not mix callsite and callee hotness based updates..

It would be good in the patch description to maybe clarify the intent / motivation here? I assume the goal is to simplify reasoning about which thresholds are triggered in which cases when the new PM is available? At least, that is my guess based on our in-person conversations.

Fri, May 12, 10:54 PM

Thu, May 11

chandlerc updated the diff for D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..

Update based on review comments.

Thu, May 11, 8:01 PM
chandlerc added a comment to D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..

The algorithm you have, I think, it's correct to start (although as pointed out by Sanjoy doesn't really compute the dominance frontier).
Have you considered computing the iterated dominance frontier every time instead? If there are technical difficulties (or it's just slower) I'd add a comment to the code explaining why we can't use it.

Thu, May 11, 7:51 PM
chandlerc committed rL302867: [PM/Unswitch] Teach the new simple loop unswitch to handle loop.
[PM/Unswitch] Teach the new simple loop unswitch to handle loop
Thu, May 11, 7:33 PM
chandlerc closed D32699: [PM/Unswitch] Teach the new simple loop unswitch to handle loop invariant PHI inputs and to rewrite PHI nodes during the actual unswitching. by committing rL302867: [PM/Unswitch] Teach the new simple loop unswitch to handle loop.
Thu, May 11, 7:33 PM
chandlerc added a comment to D32699: [PM/Unswitch] Teach the new simple loop unswitch to handle loop invariant PHI inputs and to rewrite PHI nodes during the actual unswitching..

Thanks both for the review. Landing with some fixes, but further discussion below and I'll follow-up with anything else.

Thu, May 11, 7:32 PM
chandlerc added a comment to D32819: [IR] Switch AttributeList to use an array for O(1) access.
In D32819#752793, @rnk wrote:
In D32819#749163, @rnk wrote:

One question remains: do you think I should completely encapsulate this indexing change by adding 1 inside AttributeList::getAttributes, or is it better to update all the callers as I did in this change? The problem with this change is that it will break out-of-tree backends and frontends that use the APIs with one-based indexing. It won't even be a compile failure because the APIs still take unsigned integers. That's a bit rude without some more hand waving on llvm-dev and release notes.

I'm very concerned about breaking out-of-tree code in a way that's not detectable at compile time. We have a whole bunch of out-of-tree passes that will be affected by this (...I'll fix them since I know about it) but others may not be watching the mailing lists as closely. Of course we make no guarantees about C++ API stability, but I feel we shouldn't cause unnecessary pain and breakage if it can be avoided.

Having just debugged a lot of "index out of bounds" type runtime failures, I generally agree, I don't want to put others through it. :)

How about renaming the APIs that take or return arg indices? That'll make it abundantly clear that their behavior has changed (and will also easily flag up all the places in upstream code that need to be changed).

I don't want to use bad names (hasAttributeEx?) just for backwards compatibility. One idea I had was to make a better API, one that has no attribute list indices at all, but there are a handful of places in LLVM that abstract over argument and return attributes with attribute list indices, and I couldn't find a way to replace them. Maybe rename the old APIs to hasRetOrArgAttr or something painfully ugly like that to discourage people from using them? We'd have (has|get|add|remove)ParamAttr, *RetAttr, *FnAttr to make it easier to migrate code to do the right thing.

Thu, May 11, 6:10 PM
chandlerc added a comment to D33074: InstCombine: Allow sinking instructions with more uses in the same block..

High level comment: this feels really strange to be in instcombine.

Thu, May 11, 4:04 PM
chandlerc accepted D33106: Decrease inlinecold-threshold to 45.

This seems like a strict improvement to me. I'd love to have a more thorough analysis of different values here but I don't see any reason to hold up bringing this in line with the cold callsite threshold. LGTM.

Thu, May 11, 12:03 PM
chandlerc created D33097: [x86] Follow-up to r302785 - don't widen vselect conditions when the vNi1 type being split into is a legal type (like it usually is w/ AVX-512)..
Thu, May 11, 4:53 AM
chandlerc committed rL302785: [x86] Fix a failure to select with AVX-512 when the type legalizer.
[x86] Fix a failure to select with AVX-512 when the type legalizer
Thu, May 11, 4:05 AM

Wed, May 10

chandlerc added a comment to D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently.

Trying to provide answers to the open questions here...

Wed, May 10, 5:41 PM
chandlerc added a comment to D33024: [Support] Move LLD parallel algorithms to LLVM..

LGTM on the LLVM side. Feel free to submit when you and Rui are happy with the usage pattern for this in LLD.

Wed, May 10, 4:32 PM
chandlerc committed rL302640: Revert r301950: SpeculativeExecution: Stop using whitelist for costs.
Revert r301950: SpeculativeExecution: Stop using whitelist for costs
Wed, May 10, 5:43 AM
chandlerc added a comment to D24544: SpeculativeExecution: Stop using whitelist for costs.

I'd suggest looking over the isSafeToSpeculate code carefully. If we had any bugs there, this whitelist might have been helping to cover over them in practice. In particular, the default: return true; case in that function looks suspicious. You might want to convert that into a fully covered switch and cross reference the two lists to make sure this change isn't accidentally semantic.

Perhaps unsurprisingly, this review comment proved prophetic.

This patch causes us to "miscompile" some CUDA kernels.

That said, LLVM may not be at fault as PTXAS has known bugs that miscompile valid PTX programs, so I'm going to try to somehow produce a test case that shows what is going on here.

Wed, May 10, 5:36 AM
chandlerc added a comment to D24544: SpeculativeExecution: Stop using whitelist for costs.

I'd suggest looking over the isSafeToSpeculate code carefully. If we had any bugs there, this whitelist might have been helping to cover over them in practice. In particular, the default: return true; case in that function looks suspicious. You might want to convert that into a fully covered switch and cross reference the two lists to make sure this change isn't accidentally semantic.

Wed, May 10, 5:22 AM

Tue, May 9

chandlerc committed rL302618: Update Polly for LLVM API change r302571 that removed varargs functions.
Update Polly for LLVM API change r302571 that removed varargs functions
Tue, May 9, 7:52 PM
chandlerc added a comment to D32541: Supress all uses of LLVM_END_WITH_NULL.

Forgot about polly which was broken by this. I fixed it in r302618.

Tue, May 9, 7:52 PM
chandlerc added inline comments to D33024: [Support] Move LLD parallel algorithms to LLVM..
Tue, May 9, 7:48 PM
chandlerc added a comment to D33024: [Support] Move LLD parallel algorithms to LLVM..

Maybe I'm stuck with an old diff or something?

Tue, May 9, 7:29 PM
chandlerc added a comment to D33016: [Parallel] Add support for parallel execution policies to match C++ Parallelism TS.

Other than a YAGNI argument against the type-erased stuff, seems fine to me.

Tue, May 9, 6:00 PM
chandlerc requested changes to D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently.

Somewhat focused on the store side. Near the bottom is a high-level comment about the load shrinking approach.

Tue, May 9, 5:33 PM

Mon, May 8

chandlerc added a comment to D32614: [GVNHoist] Fix: PR32821, add check for anticipability in case of infinite loops.

You are continuing to argue that you do not need to add tests because they will pass. That seems to be missing the point. Tests are what verify and validate these kinds of arguments and assumptions, both now and in the future.

Mon, May 8, 5:07 PM
chandlerc requested changes to D32614: [GVNHoist] Fix: PR32821, add check for anticipability in case of infinite loops.

I don't see what would have addressed my concerns.

Mon, May 8, 3:56 PM

Fri, May 5

chandlerc accepted D32775: Provide an invalidate method in ProfileSummaryInfo that returns false.

LGTM

Fri, May 5, 3:21 PM

Thu, May 4

chandlerc accepted D32885: [ADT] Add BitVector::find_prev.

LGTM, nice!

Thu, May 4, 5:39 PM
chandlerc accepted D32886: [asan] A clang flag to enable ELF globals-gc.

Generally makes sense. A question about the name. Feel free to land with a name that you and other sanitizer folks are happy with.

Thu, May 4, 5:38 PM
chandlerc added inline comments to D32885: [ADT] Add BitVector::find_prev.
Thu, May 4, 4:29 PM
chandlerc added a comment to D32826: Move Parallel.h from LLD to LLVM.

So, my concern about reviewing this isn't just about the implementation, it's also about the *API*. We need to make sure that the API design for parallel building blocks that we want to be available for the entire LLVM project are right. That's the particular split I was looking for...

Thu, May 4, 4:25 PM
chandlerc added a comment to D32826: Move Parallel.h from LLD to LLVM.

So, I understand that this is just moving code from LLD to LLVM, but this is pretty complex and subtle code. I think it needs really careful and thorough review. I'm going to try to plan some time for that, but I wonder -- are there any meaningful splits you can make to introduce this more incrementally to LLVM

Thu, May 4, 2:55 AM

Wed, May 3

chandlerc accepted D32768: [PM] Add ProfileSummaryAnalysis as a required pass in the new pipeline..

LGTM, excited! (But if you can, maybe wait for davide to confirm this is what he was looking for?)

Wed, May 3, 6:01 PM

Tue, May 2

chandlerc accepted D32766: [CodeGen] Don't require AA in SDAG/2Addr at CodeGenOpt::None..

LGTM, nice!

Tue, May 2, 10:40 PM
chandlerc accepted D32748: [Triple] Add a "macos" OS type that acts as a synonym for "macosx".

LGTM with David's suggested change.

Tue, May 2, 10:35 PM
chandlerc created D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass..
Tue, May 2, 3:54 AM

Mon, May 1

chandlerc created D32699: [PM/Unswitch] Teach the new simple loop unswitch to handle loop invariant PHI inputs and to rewrite PHI nodes during the actual unswitching..
Mon, May 1, 3:59 AM
chandlerc accepted D32634: Emulate TrackingVH using WeakVH.

LGTM, this looks like a nice simplification and frees up the useful bit to support non-tracking VHs. Tiny nit below, but feel free to submit this and the non-tracking weak stuff when you're ready.

Mon, May 1, 12:46 AM

Sun, Apr 30

chandlerc accepted D32691: Remove unneeded struct; NFC.

This seems trivially correct. Feel free to submit. If there is a use case, we can always add this back equally trivially when the code to use it is added...

Sun, Apr 30, 10:52 PM
chandlerc added a comment to D31924: SROA: Allow eliminating addrspacecasted allocas.

What about approaching this more from the inference perspective? Could we embed the inference into the iteration of SROA without shifting the restrictions so much?

I'm not sure exactly what you mean by this. Do you mean somehow merging InferAddressSpaces and SROA?

Sun, Apr 30, 10:39 PM
chandlerc requested changes to D32203: [SROA] Add support for non-integral pointers.

relatively minor stuff here

Sun, Apr 30, 8:50 PM
chandlerc requested changes to D32614: [GVNHoist] Fix: PR32821, add check for anticipability in case of infinite loops.

(just marking this as needing changes so it isn't on my phab dashboard)

Sun, Apr 30, 8:46 PM
chandlerc requested changes to D32634: Emulate TrackingVH using WeakVH.
Sun, Apr 30, 8:45 PM
chandlerc requested changes to D32608: SROA: Use correct address space when splitting loads (PR26154).

This test case should be cleaned up and merged into SROA/address-spaces.ll. Also, if I delete the 'getPointerAddressSpace()` call on line 3701, no test fails, so we clearly need better tests for the address space manipulation code in this file, or the call on line 3701 is actually dead and it should be *replaced* with the load-based address space in this patch.

Sun, Apr 30, 7:17 PM
chandlerc added a comment to D32614: [GVNHoist] Fix: PR32821, add check for anticipability in case of infinite loops.

Regardless of what you do with the code (I'm not an expert on this pass or the stuff Danny is already discussing there), please simplify this test case to a more readable and clear test case for the fundamental issue you're hitting. Having tons of Clang-specific bits in here really opacifies what scenario you're trying to test.

Sun, Apr 30, 6:36 PM
chandlerc added inline comments to D32634: Emulate TrackingVH using WeakVH.
Sun, Apr 30, 6:15 PM

Fri, Apr 28

chandlerc accepted D32664: [LoopUnswitch] Don't remove instructions with side effects after folding them.

This seems correct and safe. Happy for you to land as-is, or to try a different approach if you want. I don't think you're making anything worse, that's for sure. =D

Fri, Apr 28, 5:15 PM

Thu, Apr 27

chandlerc added a comment to D32409: [PM/LoopUnswitch] Introduce a new, simpler loop unswitch pass..

Thanks for the reviews, working on follow ups. Some are obvious and I'll just submit but let me know if you see anything weird.

Thu, Apr 27, 11:58 AM
chandlerc committed rL301576: [PM/LoopUnswitch] Introduce a new, simpler loop unswitch pass..
[PM/LoopUnswitch] Introduce a new, simpler loop unswitch pass.
Thu, Apr 27, 11:58 AM