chandlerc (Chandler Carruth)Administrator, Email Not Verified
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

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

First off, really sorry to keep sending *partial* code reviews. =[ I again didn't quite have enough time to do a full review of the patch (it is a bit large) but wanted to at least send out everything I have so that you aren't blocked waiting on me to produce some comments. =] I'll try again tomorrow to make more progress here although it may start to make sense for me to wait for an iteration as one of the refactorings I'm suggesting here will I think change the structure quite a bit.

Mon, Apr 24, 6:41 PM

Sun, Apr 23

chandlerc created D32409: [PM/LoopUnswitch] Introduce a new, simpler loop unswitch pass..
Sun, Apr 23, 3:50 PM

Fri, Apr 21

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

I'm still working on this, but since Wei mentioned he is looking at fixing the CandidatesToErase stuff, I wanted to go ahead and send these comments -- there is a significant one w.r.t. to the deletion stuff as well.

Fri, Apr 21, 6:09 PM
chandlerc added a comment to D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently.

Digging into the code next, but wanted to send some comments just on terminology and the documentation while I'm doing that.

Fri, Apr 21, 3:55 PM
chandlerc accepted D32367: [BitVector] Hold the underlying storage as a MutableArrayRef..

LGTM

Fri, Apr 21, 12:49 PM
chandlerc accepted D32302: [BitVector] Add find_last() and find_last_unset().

LGTM with some loop simplifications.

Fri, Apr 21, 10:35 AM

Thu, Apr 20

chandlerc added a comment to D31924: SROA: Allow eliminating addrspacecasted allocas.

Restricting addrspace cast in this way seems... really hard to get right. I still have the question Eli asked: what does this mean in the absence of inbounds? What if the address spaces have different wrapping behavior even though they have the same number of bits?

Thu, Apr 20, 10:22 PM
chandlerc accepted D32335: [PartialInliner] Fix crash when inlining functions with unreachable blocks.

LGTM as well -- no sense doing anything with the unreachable blocks.

Thu, Apr 20, 9:17 PM
chandlerc accepted D32334: [BPI] Add multiplication by scalar operators to BranchProbability.

Yeah, still LGTM. =D Tiny nit pick though...

Thu, Apr 20, 7:54 PM
chandlerc added inline comments to D32302: [BitVector] Add find_last() and find_last_unset().
Thu, Apr 20, 5:48 PM
chandlerc added a comment to D32124: [BPI] Move tail computation out of the loop. NFC.

BTW, probably we should not add the tail to any edge at all... It might be that the information that two edges have exactly the same probability is mot important than the requirement that the sum of all probabilities should be 1. it seems that sum == 1 is not a strict requirement. For example, if we have three edges with the same weight then as a result the sum of probabilities will not be equal to one. What do you think?

Thu, Apr 20, 5:39 PM
chandlerc added a comment to D32124: [BPI] Move tail computation out of the loop. NFC.

Some test added.

Thu, Apr 20, 5:39 PM
chandlerc added a comment to D30062: Estimate speedup due to inlining and use that to adjust threshold..

Sorry for the delay in collecting performance numbers. I now have some data to share. First, some details on the methodology. I used ~400 microbenchmarks used internally at Google. I built them with the following percentage values of min-speedup-for-bonus: 0%, 5%, 10%, and 15%. I ran each benchmark 10 times in each configuration. Speedup/slowdown for a benchmark is calculated only when the p-value <=0.05 (and thus the results might include different subset of benchmarks for different configs). The numbers presented below are the geomean across all benchmarks.

Config | #Benchmarks | Geomean | #Slowdowns | #Speedups | Size increase percentage
0% | 134 | 2.92% | 51 | 83 | 2.45%
5% | 121 | 1.05% | 41 | 80 | 1.58%
10% | 115 | 0.8% | 51 | 64 | 1.32%
15% | 160 | 1.03% | 44 | 116| 1.02%

Some observations:

  • The best geomean performance comes when the min-speedup-for-bonus is set to 0%. I interpret this to mean that it is generally a performance win to increase the threshold for hot callsites, and the speedup estimation is a way to control the size growth.
  • The performance when the min-speedup-for-bonus is set to 10% sits in-between that of 5% and 15%. As I mentioned above, these are not apples-to-apples comparisons beacause we compute geomean on a different set of benchmarks. Even for the same benchmark, it is possible (and it does happen) that the performance numbers are not monotonically decreasing as the min-speedup-for-bonus is increased.
  • For comparison, I calculated the size growth if we simply apply a 3X multiplier to the threshold irrespective of the callsite frequency. The size increase is 9.7%.
Thu, Apr 20, 5:10 PM
chandlerc added a comment to D32124: [BPI] Move tail computation out of the loop. NFC.

Thanks for the updated patch description! Another minor request here.

Thu, Apr 20, 12:03 AM

Wed, Apr 19

chandlerc accepted D32244: [BitVector] Add << and >> operators.

Generally this looks pretty awesome. Thanks for all of this. Minor nitpicks inline. I'd let Craig have a look as well just in case I'm not seeing anything, but good to submit if he's happy as well.

Wed, Apr 19, 5:51 PM
chandlerc added inline comments to D32244: [BitVector] Add << and >> operators.
Wed, Apr 19, 3:51 PM
chandlerc added inline comments to D32244: [BitVector] Add << and >> operators.
Wed, Apr 19, 3:38 PM

Tue, Apr 18

chandlerc committed rL300662: Revert r300657 due to crashes in stage2 of bootstraps:.
Revert r300657 due to crashes in stage2 of bootstraps:
Tue, Apr 18, 11:36 PM
chandlerc committed rL300659: Revert r300653 and r300650. The underlying commit fixes one issue with.
Revert r300653 and r300650. The underlying commit fixes one issue with
Tue, Apr 18, 10:38 PM
chandlerc accepted D32212: Create some helpers for generating bit masks.

Nice, LGTM!

Tue, Apr 18, 9:36 PM

Fri, Apr 14

chandlerc accepted D30631: [BPI] Use metadata info before any other heuristics.

Sorry I couldn't get back to it sooner, first chance I had.

Fri, Apr 14, 12:19 AM

Thu, Apr 13

chandlerc added a comment to D30521: Introduce llc/ExecuteTestCommands pass.

Currently, I really don't understand why this is the right approach...

We sometimes want to test a specific codegen API rather than a whole pass.

This adds a special pass to llc that features a minimalistic scripting language to
call some predefined API functions so we can test the API with the usual lit+FileCheck tools.

If you're actually trying to test a specific API, why aren't unittests the correct approach?

If the problem is that the unittests are hard to write, why not write libraries to make authoring the unittest easier?

I think there is something fundamental about how we write tests in llvm, and why we do not write pass tests in a unit testing framework because hey, who stops us implementing llc/opt as some functions in a unit testing library, they mostly have trivial APIs:

I think there is a beauty and we are hitting a sweet spot in the way most of our testing works in llvm: Forcing tests to be a simple human readable text file keeps things approachable and understandable. Understanding the basics of lit, llc, opt and FileCheck is easy. Having the full power of C++ in a unit test library on the other hand makes it way too easy do "fancy" stuff that is harder to understand because it happens to abstracted away in generic code and multiple layers of APIs that you have to learn and dig through when you actually have to understand a bug.

Thu, Apr 13, 4:32 PM

Wed, Apr 12

chandlerc committed rL300038: [IR] Rename the class templates for the case iterator and case handle to.
[IR] Rename the class templates for the case iterator and case handle to
Wed, Apr 12, 2:01 AM
chandlerc committed rL300035: Update Clang for an API change to LLVM's switch case iterator (it is now.
Update Clang for an API change to LLVM's switch case iterator (it is now
Wed, Apr 12, 1:25 AM
chandlerc committed rL300032: [IR] Redesign the case iterator in SwitchInst to actually be an iterator.
[IR] Redesign the case iterator in SwitchInst to actually be an iterator
Wed, Apr 12, 12:40 AM
chandlerc closed D31548: [IR] Redesign the case iterator in SwitchInst to actually be an iterator and to expose a handle to represent the actual case rather than having the iterator return a reference to itself. by committing rL300032: [IR] Redesign the case iterator in SwitchInst to actually be an iterator.
Wed, Apr 12, 12:40 AM
chandlerc added a comment to D31548: [IR] Redesign the case iterator in SwitchInst to actually be an iterator and to expose a handle to represent the actual case rather than having the iterator return a reference to itself..

Thanks for the review. Mostly done, landing with those updates. Couple of things left, but you indicated they could be follow-ups. Questions on exactly what to do with them below.

Wed, Apr 12, 12:39 AM

Tue, Apr 11

chandlerc added a comment to D31302: [CodeGen] Compute DT/LI lazily in SafeStackLegacyPass. NFC..

FWIW, I'm pretty happy with this approach especially for something triggered by an attribute like this. It is a bit gross, but doesn't seem *that* gross. Still, should see if your explanation satisfies davide. =]

Tue, Apr 11, 9:15 PM
chandlerc added a comment to D31642: [CodeGen] Add a 'NoAAResultsWrapperPass', and use it in SDAG/2Addr..

After looking at the uses of AA in both passes here, I think I like the alternative idea of making AA optional directly in the passes. Then, for the old PM, we can use something like getAnalysisIfAvailable and just not mark the analysis as required when not optimizing codegen. What do you think? Would it be helpful if I hack up a patch?

Tue, Apr 11, 9:12 PM
chandlerc accepted D31964: CodeGen: BlockPlacement: Clear ComputedEdges between functions..

LGTM. Also, this is clearly correct, feel free to land this simple of bug fix directly. =]

Tue, Apr 11, 6:29 PM

Mon, Apr 10

chandlerc added a comment to D31678: [InstCombine] Fix change flag handling to report all IR changes up to the pass manager or preseved analyses.

@chandlerc do you know of other tests that check this sort of thing or can you give more guidance. I haven't spent a lot of time in the IR optimizers until recently.

Mon, Apr 10, 3:37 PM
chandlerc added a comment to D31085: [InlineCost] Increase the cost of Switch.

This really looks like it is going in the right direction. I'm going to work on reviewing some of teh code changes a bit more closely, but I wanted to mention one other thing.

Mon, Apr 10, 3:17 PM
chandlerc added a comment to D31085: [InlineCost] Increase the cost of Switch.

Chandler, do you agree with the heuristic Hans suggested above? Even though it do not cover switches that are lowered with a mix of jump table/bit test/BTree, I think this is reasonable compromise between accuracy and cost of the hook.

Mon, Apr 10, 3:13 PM
chandlerc accepted D31890: Add support for unique_ptr<T> to dyn_cast<>.

LGTM from me, and thanks for the update around dyn_cast. Maybe wait for Mehdi or some second pair of eyes just to be sure I'm not missing anything.

Mon, Apr 10, 3:08 PM
chandlerc added inline comments to D30631: [BPI] Use metadata info before any other heuristics.
Mon, Apr 10, 2:31 PM
chandlerc accepted D31701: [BPI] Refactor post domination calculation and simple fix for ColdCall.

LGTM as well.

Mon, Apr 10, 2:27 PM
chandlerc added a comment to D31678: [InstCombine] Fix change flag handling to report all IR changes up to the pass manager or preseved analyses.

I can certainly try. What exactly were you envisioning?

Mon, Apr 10, 2:23 PM
chandlerc accepted D31704: [BPI] NFC: reorder ifs to bail out earlier.

LGTM, seems like a clear improvement in this code. See one nit about the asserts below, but feel free to submit with that addressed.

Mon, Apr 10, 2:19 PM

Thu, Apr 6

chandlerc updated the diff for D31548: [IR] Redesign the case iterator in SwitchInst to actually be an iterator and to expose a handle to represent the actual case rather than having the iterator return a reference to itself..

Rebase, enhance with better testing of const iterators, fix a couple of minor
issues, and re-clang-format everything.

Thu, Apr 6, 5:15 PM

Wed, Apr 5

chandlerc added inline comments to D31701: [BPI] Refactor post domination calculation and simple fix for ColdCall.
Wed, Apr 5, 9:33 PM
chandlerc added inline comments to D31726: AliasAnalysis: Be less conservative about volatile than atomic..
Wed, Apr 5, 4:42 PM
chandlerc added inline comments to D31704: [BPI] NFC: reorder ifs to bail out earlier.
Wed, Apr 5, 10:59 AM
chandlerc added a comment to D30631: [BPI] Use metadata info before any other heuristics.

First off, thanks for the new approach. I like this direction a lot. Some more tactical comments here.

Wed, Apr 5, 10:56 AM

Mon, Apr 3

chandlerc added a comment to D30631: [BPI] Use metadata info before any other heuristics.

Given lack of response from Chandler following the update from Serguei

Mon, Apr 3, 9:32 AM

Fri, Mar 31

chandlerc created D31548: [IR] Redesign the case iterator in SwitchInst to actually be an iterator and to expose a handle to represent the actual case rather than having the iterator return a reference to itself..
Fri, Mar 31, 1:49 PM
chandlerc added inline comments to D31531: Remove readnone from invariant.group.barrier.
Fri, Mar 31, 6:50 AM
chandlerc added a comment to D31531: Remove readnone from invariant.group.barrier.

This looks good to me but I'd like Hal to also have a look to make sure I'm not missing anything.

Fri, Mar 31, 6:30 AM

Thu, Mar 30

chandlerc added inline comments to D31236: Refactor getHostCPUName to allow testing on non-native hardware..
Thu, Mar 30, 1:29 AM

Wed, Mar 29

chandlerc added a comment to D30550: ImportArguments analysis printer.

I've chatted a bit to Thomas about this off the list and wanted to follow up here so that there wasn't radio silence.

Wed, Mar 29, 4:22 PM

Mon, Mar 27

chandlerc added a comment to D31285: [PPC] Add generated tests for all atomic operations.

Done.

I also tried to remove basic block comments. It turns out to be hard, because empty CHECK lines are not allowed. So I gave up. :P

Mon, Mar 27, 7:18 AM

Sun, Mar 26

chandlerc committed rL298808: [IR] Switch to more normal template parameter names ending in `T`.
[IR] Switch to more normal template parameter names ending in `T`
Sun, Mar 26, 7:41 AM

Mar 25 2017

chandlerc committed rL298791: [IR] Make SwitchInst::CaseIt almost a normal iterator..
[IR] Make SwitchInst::CaseIt almost a normal iterator.
Mar 25 2017, 8:01 PM
chandlerc added a comment to D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently.

Thanks a lot for working on this, first round of comments!

Mar 25 2017, 7:25 AM

Mar 24 2017

chandlerc committed rL298695: Revert r298491 and r298494 which changed Clang's handling of 'nonnull'.
Revert r298491 and r298494 which changed Clang's handling of 'nonnull'
Mar 24 2017, 2:24 AM

Mar 23 2017

chandlerc accepted D31286: [X86] Fix Stale SDNode use in X86ISelDAGtoDAG.

The fix seems trivial and looks good. Please submit to at least fix the crasher.

Mar 23 2017, 7:56 AM
chandlerc added a comment to D31263: Add option to control whether llvm-pdbdump outputs in color.

I think we need something before you commit, because as it is people will be specifying --color-output unnecessarily. Even if you say "default = on" or "default = best-effort"

OK, I came up with "Override use of color (default = isatty)", which I assume should be clear enough for typical users of this tool.

I'll let it bake tonight and submit in the morning, in case Chandler or anyone else has additional ideas.

Mar 23 2017, 3:12 AM

Mar 22 2017

chandlerc added a comment to D31246: Send ANSI color codes only to TTYs.

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

That's strange, because everyone I asked thought that the raw_ostream stuff already worked this way -- i.e. doesn't send color if the destination doesn't support it. Is there a currently accepted practice for writing FileCheck test against tools that output color? Should every such tool have an explicit -no-color option?

Possibly.... What's the failure?

I added some functionality to llvm-pdbdump and included a test for it that pipes the output of llvm-pdbdump through FileCheck. Worked on Windows, which doesn't use ANSI codes to achieve colors, but failed on Linux because the ANSI codes weren't expected.

I'm happy to add a -no-color option to llvm-pdbdump that I can use to make the test output consistent on all platforms if you think that's a better way to go.

Mar 22 2017, 11:44 AM
chandlerc added a comment to D31246: Send ANSI color codes only to TTYs.

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

That's strange, because everyone I asked thought that the raw_ostream stuff already worked this way -- i.e. doesn't send color if the destination doesn't support it. Is there a currently accepted practice for writing FileCheck test against tools that output color? Should every such tool have an explicit -no-color option?

Mar 22 2017, 11:26 AM
chandlerc added a comment to D31246: Send ANSI color codes only to TTYs.

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

Mar 22 2017, 10:02 AM
chandlerc committed rL298494: Remove an overly aggressive assert in r298491 and leave a comment.
Remove an overly aggressive assert in r298491 and leave a comment
Mar 22 2017, 3:50 AM
chandlerc committed rL298491: [nonnull] Teach Clang to attach the nonnull LLVM attribute to.
[nonnull] Teach Clang to attach the nonnull LLVM attribute to
Mar 22 2017, 2:21 AM

Mar 21 2017

chandlerc committed rL298434: Don't make unqualified calls to functions that could well be found via.
Don't make unqualified calls to functions that could well be found via
Mar 21 2017, 1:28 PM
chandlerc added a comment to D30904: Allow suppressing host and target info in VersionPrinter.

I think that the idea that you propose is good, however, I think that may be better to do as a follow up. This would get the immediate benefit for the tooling, and we can work out a nicer way to expose the information to the other users. Making it something which each app does means that we would need to change all the apps as well, which would be a much larger change.

Mar 21 2017, 1:04 PM
chandlerc added a comment to D30904: Allow suppressing host and target info in VersionPrinter.

This seems like a totally nice direction, but I wonder if I could convince you to scope creep this a bit?

Mar 21 2017, 12:16 PM
chandlerc committed rL298382: Revert r298274: "Use pthreads for thread-local lsan allocator cache on darwin".
Revert r298274: "Use pthreads for thread-local lsan allocator cache on darwin"
Mar 21 2017, 8:43 AM

Mar 17 2017

chandlerc updated the diff for D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..

Update with fixes suggested in review.

Mar 17 2017, 10:42 PM
chandlerc added a comment to D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..

Function argument order fixed.

Mar 17 2017, 10:41 PM
chandlerc added a comment to D27114: Preserve nonnull metadata on Loads through SROA & mem2reg..

I was hoping Chandler would continue to review, but I guess I can do it.

Mar 17 2017, 4:51 PM
chandlerc added a comment to D31102: Rename AttributeSet to AttributeList.

I think it'd be good to send a quick RFC / heads up email to the dev list about the refactorings you're planning here just so that folks know what the roadmap is. I'll review this in the mean time.

Mar 17 2017, 2:51 PM
chandlerc added a comment to D31080: [DAG] Extract switch lowering as a spearate object NFC.
In D31080#704197, @hans wrote:

Hi Jun,

I think extracing the logic for clustering cases into jump tables, bit tests, etc. into a separate class might be a good idea, but I don't think we should extract the actual lowering parts. If the purpose of this is to expose a hook to figure out how a switch table will be lowered, the actual lowering doesn't need to happen here. The class that deals with clustering cases should not need to know about SDAGBuilder.

Mar 17 2017, 1:38 PM

Mar 16 2017

chandlerc added inline comments to D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..
Mar 16 2017, 7:09 PM
chandlerc added a comment to D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..

Ping?

Mar 16 2017, 5:55 PM
chandlerc accepted D31058: Store Arguments in a flat array instead of an iplist.

LGTM with whatever you decide about the allocator stuff.

Mar 16 2017, 5:54 PM
chandlerc added inline comments to D31058: Store Arguments in a flat array instead of an iplist.
Mar 16 2017, 3:45 PM
chandlerc accepted D31052: Remove getArgumentList() in favor of arg_begin(), args(), etc.

LGTM once split.

Mar 16 2017, 3:44 PM
chandlerc added inline comments to D31058: Store Arguments in a flat array instead of an iplist.
Mar 16 2017, 3:29 PM
chandlerc accepted D31057: Make Argument::getArgNo() constant time, not O(#args).

This seems like a very reasonable tradeoff and a clean patch. Especially reasonable given the subsequent improvements. Ship it.

Mar 16 2017, 3:24 PM
chandlerc committed rL297940: [PM/Inliner] Fix a bug in r297374 where we would leave stale calls in.
[PM/Inliner] Fix a bug in r297374 where we would leave stale calls in
Mar 16 2017, 3:57 AM
chandlerc committed rL297935: [PM/Inliner] Add a test case that encapsulates the core issue addressed.
[PM/Inliner] Add a test case that encapsulates the core issue addressed
Mar 16 2017, 3:26 AM

Mar 12 2017

chandlerc added inline comments to D30853: Improve the genericity of `llvm::enumerate()`..
Mar 12 2017, 10:49 PM

Mar 10 2017

chandlerc accepted D30849: Use a WeakVH for UnknownInstructions in AliasSetTracker.

Nice, I like it.

Mar 10 2017, 5:04 PM
chandlerc added a comment to D30521: Introduce llc/ExecuteTestCommands pass.

Currently, I really don't understand why this is the right approach...

Mar 10 2017, 11:27 AM
chandlerc updated the diff for D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..

Update to fix subtle bugs in handling arguments. Thanks to David for the review
comments that caused me to see the issue here.

Mar 10 2017, 1:35 AM
chandlerc added inline comments to D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..
Mar 10 2017, 1:20 AM
chandlerc added a comment to D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..

Are users allowed to call these routines with a null pointer and a non-zero size? Or can we assume that if the size is known to be non-zero at compile time, the pointers are not null?

Mar 10 2017, 12:46 AM

Mar 9 2017

chandlerc created D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source code contains them..
Mar 9 2017, 7:59 PM
chandlerc committed rL297374: [PM/Inliner] Make the new PM's inliner process call edges across an.
[PM/Inliner] Make the new PM's inliner process call edges across an
Mar 9 2017, 3:47 AM

Mar 8 2017

chandlerc added a comment to D30726: Do not cache AliasSetTracker even in the old PM.

We're only caching to save on compile time, right? Is there any noticeable compile-time impact?

I'll measure.

Mar 8 2017, 6:42 PM

Mar 7 2017

chandlerc added a comment to D30728: CodeGen: Placement: Apply triangle heuristic more aggressively at O3..

So, this didn't go to the list. I'd abandon it and create a fresh revision with llvm-commits CC'ed directly.

Mar 7 2017, 5:30 PM

Mar 6 2017

chandlerc added a comment to D30651: [InlineCost, -Oz] Don't take into account the penalty of a fast call of frequently used functions.

Based on this we can expect fast calls will have minimized call overhead.

That's true, in some sense... but that's also true for the C calling convention in most cases.

If it makes no sense why functions are marked as fastcc so aggressively. I think not all calls of them can actually be made as fastcc.

It's better to think of fastcc as the default calling convention for everything where the LLVM optimizer controls the calling convention. It passes values in registers where it makes sense, and tries to strike a reasonable balance between caller-save and callee-save registers. We use it over the C calling convention where we can so that we aren't stuck doing stupid things, like passing everything on the stack on x86-32, or shuffling floating-point values between integer and VFP registers on ARM with a softfp abi. It doesn't really indicate anything about the absolute overhead of a call.

Mar 6 2017, 1:39 PM
chandlerc added a comment to D30631: [BPI] Use metadata info before any other heuristics.

In addition to the issue David pointed out, I don't understand the motivation yet.

Mar 6 2017, 1:18 PM
chandlerc requested changes to D30633: [BPI] Unreachable branch has 0 probability.

We intentionally didn't use zero because that seems more of a degenerate test case. What problem are you actually trying to solve here?

Mar 6 2017, 11:50 AM

Mar 3 2017

chandlerc committed rL296862: [SDAG] Revert r296476 (and r296486, r296668, r296690)..
[SDAG] Revert r296476 (and r296486, r296668, r296690).
Mar 3 2017, 2:15 AM

Mar 1 2017

chandlerc added a comment to D30309: CodeGen: BlockPlacement: Precompute layout for chains of triangles..

if BP is not correct, it is better to improve static branch prediction. We explicitly added a threshold for the cost based analysis result to kick in just to be conservative when the branch probability is not biased enough. Even for the long chain case, tail dup is enabled for 50/50 case, but the real profile is 40/60, taildup will hurt performance. I don't see the reason to by pass the branch prob + cost analysis by just looking at the shape.

Well, long chains amortize the penalty, so looking for the shape is definitely necessary.

I can adjust the static prediction if you'd like, but I have a source for the 60/40 stat:
See page 13 here:
http://digitalassets.lib.berkeley.edu/techreports/ucb/text/CSD-83-121.pdf

Mar 1 2017, 12:23 PM

Feb 28 2017

chandlerc committed rL296444: [IR] Add range accessors for the indices of a GEP instruction..
[IR] Add range accessors for the indices of a GEP instruction.
Feb 28 2017, 12:16 AM

Feb 27 2017

chandlerc accepted D30434: [LCG] Fix EXPENSIVE_CHECKS typo. NFC.

LGTM as well. Lemme know if you'd like me to land.

Feb 27 2017, 3:59 PM

Feb 26 2017

chandlerc added inline comments to D30062: Estimate speedup due to inlining and use that to adjust threshold..
Feb 26 2017, 5:10 PM
chandlerc added inline comments to D30062: Estimate speedup due to inlining and use that to adjust threshold..
Feb 26 2017, 3:02 PM

Feb 24 2017

chandlerc added a comment to D30062: Estimate speedup due to inlining and use that to adjust threshold..

Some initial (minor) comments. It'd also help I think to rebase this as some of the refactorings land.

Feb 24 2017, 12:34 PM
chandlerc added a comment to D30108: Refactor code computing switch instruction cost. NFC..

There's nothing wrong with this code, but this is a refactoring that I think only makes sense in the context where you need it. As-is, it seems like unnecessarily more layers with no real benefit. Let's look at this in the end-state patch?

Feb 24 2017, 12:20 PM