chandlerc (Chandler Carruth)Administrator
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Dec 8

chandlerc added inline comments to D40987: Rewrite the cached map used for locating the most precise DIE among inlined subroutines for a given address..
Fri, Dec 8, 11:53 AM
chandlerc added a comment to D40987: Rewrite the cached map used for locating the most precise DIE among inlined subroutines for a given address..

Thanks for all the feedback!

Fri, Dec 8, 3:16 AM
chandlerc updated the diff for D40987: Rewrite the cached map used for locating the most precise DIE among inlined subroutines for a given address..

Update based on code review comments.

Fri, Dec 8, 3:16 AM
chandlerc requested changes to D40648: A few initializations to please Valgrind. NFC.

If MSan doesn't complain about these, I'm inclined to leave them uninitialized. I'm not sure how valuable it is to workaround valgrind false-positives.

Fri, Dec 8, 2:31 AM

Thu, Dec 7

chandlerc created D40987: Rewrite the cached map used for locating the most precise DIE among inlined subroutines for a given address..
Thu, Dec 7, 3:24 PM

Tue, Nov 28

chandlerc committed rL319164: Add a new pass to speculate around PHI nodes with constant (integer) operands….
Add a new pass to speculate around PHI nodes with constant (integer) operands…
Tue, Nov 28, 3:33 AM
chandlerc closed D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable. by committing rL319164: Add a new pass to speculate around PHI nodes with constant (integer) operands….
Tue, Nov 28, 3:32 AM
chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Thanks for all the feedback, landing with suggested fixes.

Tue, Nov 28, 3:16 AM
chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

LGTM

I've also only implemented this in the new pass manager. If folks are

very interested, I can try to add it to the old PM as well, but I didn't
really see much point (my use case is already switched over to the new
PM).

Isn't the legacy PM still the default we use for most frontends including clang and the new PM is only used for (Thin)LTO so far?

Tue, Nov 28, 3:05 AM

Mon, Nov 20

chandlerc added inline comments to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..
Mon, Nov 20, 2:31 PM
chandlerc updated the diff for D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Update to address the issue highlighted by David and add more test cases.

Mon, Nov 20, 2:30 PM
chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Without having read the code yet:

We could in theory make it part of the codegen pass
pipeline, but there doesn't really seem to be a good reason for that --
it isn't "lowering" in any sense and only relies on pretty standard cost
model based TTI queries, so it seems to fit well with the "optimization"
pipeline model.

This does sound like a CodeGen pass to me! Not all of CodeGen is lowering, we do have optimization passes there too. And the fact that TTI is used is another indicator that this is a machine specific pass. This being part of CodeGen would also allow targets to not add the pass to the pipeline if it doesn't help them.

Mon, Nov 20, 2:22 PM

Fri, Nov 17

chandlerc committed rL318549: [PM/Unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching,.
[PM/Unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching,
Fri, Nov 17, 12:00 PM
chandlerc closed D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple. by committing rL318549: [PM/Unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching,.
Fri, Nov 17, 12:00 PM

Wed, Nov 15

chandlerc added a comment to D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple..

Thanks Davide, I think I've got these covered either by adjustments or future work. Give a shout about anything you'd still like addresed in follow-up patches.

Wed, Nov 15, 11:06 PM

Tue, Nov 14

chandlerc accepted D40007: [NewPassManager] Pass the -fdebug-pass-manager flag setting into the Analysis managers to match what we do in opt.

LGTM, nice catch!

Tue, Nov 14, 12:11 AM

Mon, Nov 13

chandlerc committed rL318137: [PM] Require a registered x86 target for this test which uses the x86.
[PM] Require a registered x86 target for this test which uses the x86
Mon, Nov 13, 9:20 PM
chandlerc committed rL318131: [PM] Wire up support for the bounds checking sanitizer with the new PM..
[PM] Wire up support for the bounds checking sanitizer with the new PM.
Mon, Nov 13, 6:00 PM
chandlerc closed D39085: [PM] Wire up support for the bounds checking sanitizer with the new PM. by committing rL318131: [PM] Wire up support for the bounds checking sanitizer with the new PM..
Mon, Nov 13, 6:00 PM
chandlerc committed rL318130: [PM] Add a missing header that I had in the next commit but was needed.
[PM] Add a missing header that I had in the next commit but was needed
Mon, Nov 13, 5:48 PM
chandlerc committed rL318128: [PM] Port BoundsChecking to the new PM..
[PM] Port BoundsChecking to the new PM.
Mon, Nov 13, 5:32 PM
chandlerc closed D39084: [PM] Port BoundsChecking to the new PM. by committing rL318128: [PM] Port BoundsChecking to the new PM..
Mon, Nov 13, 5:32 PM
chandlerc added a comment to D39084: [PM] Port BoundsChecking to the new PM..

Thanks, landing with comments addressed.

Mon, Nov 13, 5:28 PM
chandlerc committed rL318124: [PM] Refactor BoundsChecking further to prepare it to be exposed both as.
[PM] Refactor BoundsChecking further to prepare it to be exposed both as
Mon, Nov 13, 5:14 PM
chandlerc closed D39081: [PM] Refactor BoundsChecking further to prepare it to be exposed both as a legacy and new PM pass. by committing rL318124: [PM] Refactor BoundsChecking further to prepare it to be exposed both as.
Mon, Nov 13, 5:14 PM
chandlerc added a comment to D39783: Add ADL support to range based <algorithm> extensions.

Rather than adding llvm-commits to an old revision, please abandon the revision and create a fresh one with the patch and llvm-commits at the start. That way email gets sent to the list with the full description of the patch. Thanks.

Mon, Nov 13, 4:53 PM

Nov 9 2017

chandlerc accepted D39855: [ADT] Rewrite mapped_iterator in terms of iterator_adaptor_base..

LGTM

Nov 9 2017, 3:24 PM

Nov 7 2017

chandlerc added inline comments to D39111: Extensible LLVM RTTI.
Nov 7 2017, 3:02 PM

Oct 26 2017

chandlerc requested changes to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Few general concerns.

1/  Always submit patch with few test cases (valid for any patch).
2/  Patch https://reviews.llvm.org/D38494 is taking care of this problem. I don't see what is your point in duplicating the efforts.

You are on the review list also.

There is no derth of open problems to tackle. I hope you understand my point here.

Thanks.

Because we are solving issue as another way.

Oct 26 2017, 6:37 PM
chandlerc added a comment to D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior.

As with Davide, my argument was mostly "use an existing bucket" not "it should be in bucket X". =]

Oct 26 2017, 5:37 PM
chandlerc added a comment to D38824: [X86] Synchronize the existing CPU predefined macros with the cases that gcc defines them.

Where is the best place to document this policy so people have a chance of understanding it going forward?

Oct 26 2017, 3:57 PM
chandlerc accepted D39349: [X86] Make -march=i686 an alias of -march=pentiumpro.

LGTM, nice.

Oct 26 2017, 3:44 PM
chandlerc added a comment to D38824: [X86] Synchronize the existing CPU predefined macros with the cases that gcc defines them.

So, doing research to understand the impact of this has convinced me we *really* need to stop doing this. Multiple libraries are actually trying to enumerate every CPU that has feature X for some feature X. =[[[ This, combined with the fundamental pattern of defining a precise macro for the CPU, leaves a time bomb where anyone that passes a new CPU to -march using some older headers will incorrectly believe features aren't available on *newer* CPUs. =[

Oct 26 2017, 1:50 PM

Oct 25 2017

chandlerc added a comment to D39111: Extensible LLVM RTTI.

The virtual call could be removed, perhaps, by having the base class hold a non-static member void*...

Oh that's clever. I hadn't thought of that. There is a space trade-off though as it would require one char per RTTI object (in addition to the current requirement of one BSS char per class).

Oct 25 2017, 4:17 PM
chandlerc requested changes to D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior.

I don't really like the ever growing set of configuration macros here.

Oct 25 2017, 3:43 PM

Oct 19 2017

chandlerc created D39085: [PM] Wire up support for the bounds checking sanitizer with the new PM..
Oct 19 2017, 2:19 AM
chandlerc added a dependent revision for D39084: [PM] Port BoundsChecking to the new PM.: D39085: [PM] Wire up support for the bounds checking sanitizer with the new PM..
Oct 19 2017, 2:19 AM
chandlerc created D39084: [PM] Port BoundsChecking to the new PM..
Oct 19 2017, 2:18 AM
chandlerc created D39081: [PM] Refactor BoundsChecking further to prepare it to be exposed both as a legacy and new PM pass..
Oct 19 2017, 1:09 AM

Oct 18 2017

chandlerc committed rL316135: [PM] Refactor the bounds checking pass to remove a method only called in.
[PM] Refactor the bounds checking pass to remove a method only called in
Oct 18 2017, 3:42 PM

Oct 17 2017

chandlerc added inline comments to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..
Oct 17 2017, 11:16 AM
chandlerc updated the diff for D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Add a test case covering critical edge splitting.

Oct 17 2017, 11:15 AM
chandlerc added inline comments to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..
Oct 17 2017, 3:26 AM
chandlerc updated the diff for D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Add a stricter form of checking for profitability to avoid one potential issue
with this patch. Also clean up comments in various places to try and help
address code review feedback.

Oct 17 2017, 3:24 AM
chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Hopefully responding to all of the comments. Note the last one which is probably the most interesting point of discussion around cost modeling.

Oct 17 2017, 12:03 AM

Oct 16 2017

chandlerc updated the diff for D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple..

Update addressing the remaining comments from Sanjoy.

Oct 16 2017, 1:03 AM
chandlerc added inline comments to D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple..
Oct 16 2017, 1:02 AM
chandlerc added a comment to D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple..

Updates and/or responses. Largely addressed all the comments with the next patch upload.

Oct 16 2017, 12:52 AM

Oct 15 2017

chandlerc updated the diff for D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple..

Rebase onto the new LoopInfo API and get everything working. Also rebase into
a monorepo checkout.

Oct 15 2017, 12:31 AM

Oct 13 2017

chandlerc added a comment to D38085: Use the basic cost if a GEP is not used as addressing mode.

This patch got reverted again, and I wanted to provide some insight as to why...

Oct 13 2017, 11:00 AM

Oct 12 2017

chandlerc added a comment to D38489: TargetMachine: Merge TargetMachine and LLVMTargetMachine.

Turns out this doesn't work nice with the current layering. There are some tools that just link libTarget but not libCodeGen. Merging LLVMTargetMachine and TargetMachine however forces us to move TargetMachine into libCodeGen breaking such users.

I think when merging LLVMTargetMachine and TargetMachine we also have to be consequent and merge libTarget into libCodeGen to facilitate that. Not sure how people think about that, and I'm not sure I will find the time to prepare patches for all llvm users linking to libTarget.

Oct 12 2017, 6:23 PM

Oct 11 2017

chandlerc added inline comments to D38824: [X86] Synchronize the existing CPU predefined macros with the cases that gcc defines them.
Oct 11 2017, 4:12 PM
chandlerc added inline comments to D38824: [X86] Synchronize the existing CPU predefined macros with the cases that gcc defines them.
Oct 11 2017, 3:18 PM

Oct 10 2017

chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

so it is possible that the large improvement in compile time is due to we broke the pipeline and due to this less optimization was possible/executed.

Oct 10 2017, 11:48 PM
chandlerc added a comment to D38433: Introduce a specialized data structure to be used in a subsequent change.

(Haven't addressed the code comments yet since the design isn't settled)

Have you considered building a ChunkedVector instead of a ChunkedList? Specifically, there is a great trick where you use a single index with the low bits being an index into the chunk and the high bits being an index into a vector of pointers. It has many of the benefits you list and is a bit simpler I think. It also supports essentially the entire vector API if desired. Both bi-directional and even random access are reasonably efficient. Good locality, etc.

With a vector-of-buffers implementation, I'm a bit worried about the space overhead on the smaller cases. For instance, this is the histogram of how this data structure is populated from a clang-bootstrap (also in https://reviews.llvm.org/D38434):

     Count: 731310
       Min: 1
      Mean: 8.555150
50th %tile: 4
95th %tile: 25
99th %tile: 53
       Max: 433

If I used a vector-of-buffers, I will either have to recompute the capacity and end (of the last buffer) on every insert (which will require an additional deref and some computation) or have to keep two words in the data structure over the three that smallvector keeps anyway. This adds a lot of relative overhead on the median case (4 elements). In fact, the current situation of two extra words also qualifies as a "lot" of relative overhead IMO, and I want to think of an SSO to improve the situation.

Oct 10 2017, 1:20 PM
chandlerc added a comment to D38433: Introduce a specialized data structure to be used in a subsequent change.

Have you considered building a ChunkedVector instead of a ChunkedList? Specifically, there is a great trick where you use a single index with the low bits being an index into the chunk and the high bits being an index into a vector of pointers. It has many of the benefits you list and is a bit simpler I think. It also supports essentially the entire vector API if desired. Both bi-directional and even random access are reasonably efficient. Good locality, etc.

Oct 10 2017, 10:31 AM

Oct 7 2017

chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

when this was merged in to rust I noticed that it did not give any gain.
I think that I made some thing wrong when I tested your modification from my original patch.
as I understand it is to late at this location due to the most time is spent "in" addFunctionSimplificationPasses.

what was the problem with having the GDCE pass directly after the add of the inliner?

Oct 7 2017, 4:12 PM

Oct 5 2017

chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

(quick responses, still working on code updates from the review)

Oct 5 2017, 6:08 PM
chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

I've taken some time to go through the code in NewGVN that computes similar things based on the suggestion from Danny. These now do *very* similar things in terms of walk. The differences I've seen are:

Oct 5 2017, 10:06 AM
chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

I would've checked in this a while ago :) Anyway, I think it was good to run some tests, but this is clearly a good thing to do.
I'm going ahead and committing this as it seems we built enough consensus around it. In the worst case, we're going to revert it :)

Oct 5 2017, 9:58 AM
chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

I did a small test in rust with -Os and the compile time do not change with the patch so a check for OptLevel > 2 is maybe good to have.

Oct 5 2017, 12:54 AM

Oct 4 2017

chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

FWIW, I think with the latest patch update this has now addressed Danny's primary concerns, and should be much more effective at avoiding re-visiting regions of IR.

Oct 4 2017, 2:38 AM
chandlerc updated the diff for D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Rebase (and move to mono-repo layout, sorry for any disruption).

Oct 4 2017, 2:36 AM

Oct 3 2017

chandlerc added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

I'll admit to not staring at the cost model too hard (I rarely have useful input on that kind of target specific thing), but it looked at a glance like you trying to calculate which might constant fold as well.
If not, or it's not part of the goal, awesome.

Oct 3 2017, 7:09 PM
chandlerc added a reviewer for D38359: [X86] Ignore DBG instructions in X86CmovConversion optimization to resolve PR34565: rnk.

Thanks Chandler for reviewing the code.

Is there any way that the debug instructions can become invalid due to this transform? I'm not seeing anything obvious, but wanted to make sure you've thought about this too.

This is how the code I am trying to fix looks like:

%vreg2<def,tied1> = CMOVB32rr %vreg6<tied0>, %vreg1, %EFLAGS<imp-use>
DBG_VALUE %vreg2, %noreg, !"b", <!DIExpression()>; GR32:%vreg2 line no:7
DBG_VALUE %vreg2, %noreg, !"b", <!DIExpression()>; GR32:%vreg2 line no:7
%vreg3<def,tied1> = CMOVB32rr %vreg0<tied0>, %vreg6, %EFLAGS<imp-use>

I assume 3 facts at this point:

  1. DBG_VALUE has only virtual register and cannot have physical register, that is true because we are still working with in SSA machine code.
  2. DBG_VALUE instruction generates a void value, i.e., no instruction can use it (or refer to it).
  3. When we replace all uses of the CMOV instruction with the new PHINode instruction, that also changes the virtual register in the DBG_VALUE.

    Saying that, we can always move all DBG_VALUE instructions forward up to the end of Machine Basic Block, right?
Oct 3 2017, 12:54 AM

Oct 2 2017

chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

I'd recommend using LNT,

This seems like a really cumbersome process to expect folks to go through for changes intending to improve compile time. Is there no way to simplify it?

Which part is cumbersome?

Oct 2 2017, 10:23 PM
chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

I'd recommend using LNT,

Oct 2 2017, 10:05 PM
chandlerc added a reviewer for D38482: TargetMachine: Use LLVMTargetMachine in CodeGen: craig.topper.
  1. If in #1 you mean actual targets -- are there any left now that the C backend is gone? What are these targets? I wonder if we could simplify things by collapsing a no longer necessary layer.
  • There is none in open source, and I don't know of any internal ones at my company.
  • According to the mailing list this may be good for SPIR-V (http://lists.llvm.org/pipermail/llvm-dev/2017-June/114593.html) which doesn't really need any of the CodeGen infrastructure.
  • Indeed collapsing the two classes would slightly simplify the code, and I'd be perfectly fine with that.
  • On the other hand it's kinda neat to have a CodeGen internal interface with LLVMTargetMachine and an outside interface with TargetMachine
Oct 2 2017, 7:40 PM
chandlerc added a comment to D38359: [X86] Ignore DBG instructions in X86CmovConversion optimization to resolve PR34565.

Is there any way that the debug instructions can become invalid due to this transform? I'm not seeing anything obvious, but wanted to make sure you've thought about this too.

Oct 2 2017, 6:21 PM
chandlerc added inline comments to D38154: [PassManager] Run global opts after the inliner.
Oct 2 2017, 6:02 PM
chandlerc added a comment to D38482: TargetMachine: Use LLVMTargetMachine in CodeGen.

I generally like the direction of the cleanup. A few high level questions:

Oct 2 2017, 5:57 PM
chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

I'm generally fine with this going in given the measurements.

Oct 2 2017, 5:31 PM

Oct 1 2017

chandlerc added inline comments to D38154: [PassManager] Run global opts after the inliner.
Oct 1 2017, 10:45 PM

Sep 27 2017

chandlerc accepted D38201: Use a BumpPtrAllocator for Loop objects.

LGTM!

Sep 27 2017, 5:58 PM

Sep 26 2017

chandlerc added a comment to D38201: Use a BumpPtrAllocator for Loop objects.

There is a problem with this patch -- the legacy pass manager checks if a loop is valid or not by querying isInvalid, which is incorrect; since we call the destructor for loop objects to mark them as "invalid" calling isInvalid for possibly invalid loop objects has UB. We already have this UB in User::operator delete though, and so given that and that the legacy pass manager is going away at some point, perhaps this is okay? Alternatively, I could add a mechanism on the legacy loop pass manager that lets passes denote loop objects to have been deleted (by remembering the pointers in a set) and use that to decide if a Loop object is valid or not (and with that, I can guard isInvalid with #ifndef NDEBUG).

Sadly, I think this would be problematic. I think it'll trip ASan's use-after-scope or use-after-lifetime checks.... =/ But maybe there is a way around that. Or maybe just implement the solution you suggest. Dunno, but definitely worth checking.

We already do this for User instances -- we read from fields in the User object to be destroyed in its operator delete, after the destructors have run. However, two wrongs don't make a right so I'll first try to see if I can fix this in the old pass manager.

Sep 26 2017, 11:29 PM
chandlerc accepted D37198: [InlineCost] add visitSelectInst().

(Marking as accepted now that Easwaran has had a chance to look as well)

Sep 26 2017, 11:24 PM
chandlerc added a comment to D37198: [InlineCost] add visitSelectInst().

This is looking pretty good. Some nit-picky improvements below. I'd love for Easwaran to get another chance to look at this too though...

Sep 26 2017, 3:29 PM
chandlerc added a comment to D38201: Use a BumpPtrAllocator for Loop objects.

There is a problem with this patch -- the legacy pass manager checks if a loop is valid or not by querying isInvalid, which is incorrect; since we call the destructor for loop objects to mark them as "invalid" calling isInvalid for possibly invalid loop objects has UB. We already have this UB in User::operator delete though, and so given that and that the legacy pass manager is going away at some point, perhaps this is okay? Alternatively, I could add a mechanism on the legacy loop pass manager that lets passes denote loop objects to have been deleted (by remembering the pointers in a set) and use that to decide if a Loop object is valid or not (and with that, I can guard isInvalid with #ifndef NDEBUG).

Sep 26 2017, 3:29 PM

Sep 22 2017

chandlerc requested changes to D37198: [InlineCost] add visitSelectInst().

Mostly minor suggestions below...

Sep 22 2017, 11:16 AM

Sep 21 2017

chandlerc added inline comments to D37198: [InlineCost] add visitSelectInst().
Sep 21 2017, 10:15 PM
chandlerc added a comment to D37902: [CodeExtractor] Fix multiple bugs under certain shape of extracted region.

Can you revert the unrelated formatting changes? Otherwise reviewing this is a bit hard.

Sep 21 2017, 4:06 PM
chandlerc added a comment to D38154: [PassManager] Run global opts after the inliner.

Could you include analogous updates to the new PM's pipeline? (I can do it myself in a follow-up if you'd rather...)

Sep 21 2017, 2:28 PM

Sep 20 2017

chandlerc updated the diff for D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Rebase (no substantive changes yet...)

Sep 20 2017, 12:31 AM

Sep 19 2017

chandlerc accepted D38055: Tighten the invariants around LoopBase::invalidate.

Generally looks good. Description should likely be updated to reflect changes made during review. Feel free to submit!

Sep 19 2017, 6:57 PM
chandlerc edited reviewers for D37900: [SROA] Really remove associated dbg.declare when removing dead alloca, added: rnk; removed: chandlerc.

I suspect Reid is in a much better position to look at debug info changes to SROA than I am, but let me know if I can help...

Sep 19 2017, 6:28 PM
chandlerc requested changes to D37939: [Mem2Reg] Also handle memcpy.

(marking as needing changes to clear dashboard)

Sep 19 2017, 6:27 PM
chandlerc requested changes to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

Marking as needing changes to clear dashboard.

Sep 19 2017, 6:26 PM
chandlerc removed a reviewer for D38068: [DebugInfo] Use a DenseMap to coalesce MachineOperand locations: chandlerc.
Sep 19 2017, 6:26 PM
chandlerc requested changes to D37198: [InlineCost] add visitSelectInst().

Just wanted to say thanks so much for working on this -- its a huge improvement to the inline cost.

Sep 19 2017, 6:15 PM
chandlerc requested changes to D33946: [InlineCost] Find identical loads in the callee.

Sorry for the awful delay when I got pulled into stuff, should be able to stay with the (small) stuff left in the review.

Sep 19 2017, 6:01 PM
chandlerc requested changes to D38055: Tighten the invariants around LoopBase::invalidate.
Sep 19 2017, 5:34 PM
chandlerc added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

Absolutely. I think the verifier should never, under any circumstances, mutate the IR. Think about it, with the current design if a pass corrupts the debug info the verifier may "hide" this by stripping it out rather than allowing us to find it.

Okay, I think I agree. Before I venture off implementing this, do you think that separating out a StripBrokenDebugInfoPass that depends on the Verifier is right way forward?

Sep 19 2017, 4:06 PM
chandlerc added a comment to D38055: Tighten the invariants around LoopBase::invalidate.

Generally like the API changes here. The enum seems a big improvement.

Sep 19 2017, 3:59 PM
chandlerc accepted D37996: [LoopInfo] Make LoopBase and Loop destructors non-public.

LGTM, nits below.

Sep 19 2017, 2:49 PM
chandlerc added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

That was a purely pragmatic decision: Most (but not all) LLVM-based tools are running the Verifier as an LLVM pass so adding the stripping into the pass was the least invasive way of implementing this feature. If we are truly bothered by this, I think what could work is to separate out a second StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to run immediately after it. I don't see this adding much value though, and we would have to modify all tools to schedule the new pass explicitly. Do you think that would be worth pursuing?

Sep 19 2017, 11:39 AM
chandlerc added a comment to D37939: [Mem2Reg] Also handle memcpy.

Hi @efriedma,

this is the julia pass pipeline (https://github.com/JuliaLang/julia/blob/master/src/jitlayers.cpp#L148). IIRC the original list of passes came from VMKit,
but the pass list was adjusted as needed over the years.

Sep 19 2017, 10:48 AM
chandlerc added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Sep 19 2017, 10:47 AM
chandlerc added a comment to D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule..

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

Sep 19 2017, 10:30 AM

Sep 15 2017

chandlerc committed rL313435: [git] Update the llvm git helper script to work correctly with the.
[git] Update the llvm git helper script to work correctly with the
Sep 15 2017, 7:15 PM
chandlerc committed rL313409: [SLP] Revert r312791 and other necessary commits, except for TTI and.
[SLP] Revert r312791 and other necessary commits, except for TTI and
Sep 15 2017, 3:25 PM