mehdi_amini (Mehdi AMINI)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 30 2013, 5:34 PM (224 w, 6 d)

Recent Activity

Sun, Aug 20

mehdi_amini added inline comments to D36910: [ThinLTO] WIP Tracking of per-call mod ref info.
Sun, Aug 20, 10:33 AM

Thu, Aug 17

mehdi_amini added a comment to D34992: Emit static constexpr member as available_externally definition.

Bi-weekly ping! (@rsmith)

Thu, Aug 17, 8:29 AM
mehdi_amini accepted D36673: Addressed some security issues in Dockerfiles..

LGTM

Thu, Aug 17, 8:17 AM

Mon, Aug 14

mehdi_amini added a comment to D36713: [libc++] Add a persistent way to disable availability.

Seems fine to me. Adding @dexonsmith (I don't know who maintain libc++ at Apple right now)

Mon, Aug 14, 3:25 PM
mehdi_amini updated subscribers of D36713: [libc++] Add a persistent way to disable availability.
Mon, Aug 14, 3:24 PM

Wed, Aug 9

mehdi_amini updated subscribers of D36556: [IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level.

Is this something that is affecting clang-5.0 backward compatibility with clang-4.0?
(adding @hansw for visibility)

Wed, Aug 9, 10:41 PM
mehdi_amini updated subscribers of D36556: [IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level.
Wed, Aug 9, 10:40 PM

Tue, Aug 8

mehdi_amini accepted D33513: [ThinLTO] Fix ThinLTO crash while destroying context.

LGTM.

Tue, Aug 8, 10:02 PM · lld

Mon, Aug 7

mehdi_amini added inline comments to D36440: [ThinLTO] Fix thinLTO crash .
Mon, Aug 7, 10:06 PM

Sat, Aug 5

mehdi_amini accepted D33513: [ThinLTO] Fix ThinLTO crash while destroying context.
Sat, Aug 5, 4:03 PM · lld

Fri, Aug 4

mehdi_amini added inline comments to D34992: Emit static constexpr member as available_externally definition.
Fri, Aug 4, 2:18 AM
mehdi_amini updated the diff for D34992: Emit static constexpr member as available_externally definition.

Address Richard's comment

Fri, Aug 4, 2:17 AM

Thu, Aug 3

mehdi_amini added a comment to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.

I'll leave the final approval to Teresa.

Thu, Aug 3, 4:48 PM
mehdi_amini added inline comments to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.
Thu, Aug 3, 4:45 PM
mehdi_amini added a comment to D36288: Use profile summary to disable peeling for huge working sets.

When the working set size is determined to be huge, disable peeling to avoid bloating the working set further.

Thu, Aug 3, 4:43 PM
mehdi_amini added a comment to D34992: Emit static constexpr member as available_externally definition.

Ping again @rsmith (or anyone else)

Thu, Aug 3, 4:41 PM
mehdi_amini added inline comments to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.
Thu, Aug 3, 8:32 AM

Wed, Aug 2

mehdi_amini added a comment to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.

What makes them fail?

They're missing the new field I added to the bitcode, so they fail a bounds-check assert.

Wed, Aug 2, 11:12 AM
mehdi_amini added inline comments to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.
Wed, Aug 2, 11:11 AM
mehdi_amini added a comment to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.

Currently, two tests are failing: ThinLTO/X86/autoupgrade.ll and ThinLTO/X86/drop-debug-info.ll, since they have blobs with old versions of the bitcode. Should I update those as well?

Wed, Aug 2, 11:05 AM

Mon, Jul 31

mehdi_amini added a comment to D36059: [memops] Add a new pass to inject fast-path code for specific library function calls..

Just want to point out that we're getting a bit far afield of this patch. Would love any comments on the actually technique...

Mon, Jul 31, 10:44 PM
mehdi_amini added a comment to D36059: [memops] Add a new pass to inject fast-path code for specific library function calls..

It seems to me to conceptually belong to the backend. Why isn't this part of CodeGenPrepare (or injected by the target as part of its pre-ISel IR passes)?

It definitely is similar to CGP.

The reason I didn't put it there after talking to folks (mostly Hal I think) was because I generally operate under the principle of "if it doesn't need to be in CGP, it should be separate" for maintenance, testing, etc. The usual case which necessitates transforms being in CGP is needing to participate in its iterative process, but that isn't true here. A common practical reason is because the logic is too small or isolated to really make sense as its own pass, but that doesn't seem to be true here as well.

This is my opinion as well (as I think I expressed on IRC). CGP has accumulated a lot of different pieces of functionality because it makes sense for them to iterate (similar to why InstCombine has gotten that way). I see no reason for this to be part of that iterative scheme, and so it can be a separate pass (and, thus, it should be).

Mon, Jul 31, 10:11 PM

Sun, Jul 30

mehdi_amini added inline comments to D32471: IR: Use pointers instead of GUIDs to represent edges in the module summary. NFCI..
Sun, Jul 30, 2:09 PM
mehdi_amini added a comment to D36059: [memops] Add a new pass to inject fast-path code for specific library function calls..

It seems to me to conceptually belong to the backend. Why isn't this part of CodeGenPrepare (or injected by the target as part of its pre-ISel IR passes)?

Sun, Jul 30, 10:36 AM

Wed, Jul 26

mehdi_amini added a comment to D33900: Print registered targets in clang's version information.
In D33900#820281, @hans wrote:

I think @thakis is right: this too verbose to be the default --version.
We likely shouldn't ship this in clang-5.0 (@hans).

Let me know if you figure out a solution here and I'll merge it.

Wed, Jul 26, 6:21 PM

Tue, Jul 25

mehdi_amini added a comment to D35875: ThinLTO: Don't import aliases of any kind (even linkonce_odr).

they had to be emitted linkonce_odr in all the destination modules (even if they weren't used by an alias) rather than as available_externally - causing extra object size.

Tue, Jul 25, 9:03 PM

Mon, Jul 24

mehdi_amini added a comment to D34992: Emit static constexpr member as available_externally definition.

Weekly ping! (@rsmith)

Mon, Jul 24, 12:30 PM
mehdi_amini added a comment to D33900: Print registered targets in clang's version information.

Here is the current output:

Mon, Jul 24, 8:53 AM
mehdi_amini added a reviewer for D33900: Print registered targets in clang's version information: hans.
Mon, Jul 24, 8:50 AM

Jul 21 2017

mehdi_amini added inline comments to D35633: [ThinLTO] Add FunctionAttr NoRecurse and ReadAttr propagation to ThinLTO.
Jul 21 2017, 9:21 PM
mehdi_amini added inline comments to D35702: [LTO][ThinLTO] Use the linker resolutions to mark global values as dso_local..
Jul 21 2017, 9:10 PM

Jul 20 2017

mehdi_amini added a comment to D35633: [ThinLTO] Add FunctionAttr NoRecurse and ReadAttr propagation to ThinLTO.

Thanks, that's a very good start! Few comments inline, I didn't understand everything.

Jul 20 2017, 10:27 PM
mehdi_amini added a comment to D35702: [LTO][ThinLTO] Use the linker resolutions to mark global values as dso_local..

Thanks, I'm glad to see this coming!

Jul 20 2017, 10:05 PM
mehdi_amini added inline comments to D35639: [LTO] Prevent dead stripping and internalization of symbols with sections.
Jul 20 2017, 3:54 PM
mehdi_amini added inline comments to D35639: [LTO] Prevent dead stripping and internalization of symbols with sections.
Jul 20 2017, 3:20 PM
mehdi_amini added inline comments to D35639: [LTO] Prevent dead stripping and internalization of symbols with sections.
Jul 20 2017, 2:46 PM
mehdi_amini added a comment to D35418: Handle clang-tools-extra project in docker scripts..

Oh great you just committed :) We were on the same line! I just wanted to make sure you were not waiting for me. Thanks.

Jul 20 2017, 1:34 AM
mehdi_amini added a comment to D35418: Handle clang-tools-extra project in docker scripts..

Note: I approved in the first place because my comment was so minor that you could fix it (or not) and move on with committing directly without another round of review.

Jul 20 2017, 1:34 AM

Jul 17 2017

mehdi_amini added a comment to D34992: Emit static constexpr member as available_externally definition.

@rsmith: post-C++-commitee-meeting ping ;)

Jul 17 2017, 11:39 AM

Jul 14 2017

mehdi_amini accepted D35436: [ThinLTO] Ensure we always select the same function copy to import.

LGTM.

Jul 14 2017, 1:58 PM
mehdi_amini accepted D35418: Handle clang-tools-extra project in docker scripts..

LGTM. Thanks.

Jul 14 2017, 9:28 AM
mehdi_amini added inline comments to D33946: [InlineCost] Find identical loads in the callee.
Jul 14 2017, 9:25 AM

Jul 13 2017

mehdi_amini added a comment to D35081: [ThinLTO] Allow multiple summary entries..

My first option was if any copy is under the threshold, simply pick the prevailing copy (it may be over threshold, but assume we want that one anyway). Another possibility is to only allow importing of the prevailing copy, if it is over the inst limit, too bad. The main reason I think that could be better is when we have PGO - the prevailing copy would for sure have the matched PGO data, but the non-prevailing copies may not.

Jul 13 2017, 12:02 PM
mehdi_amini added a comment to D35081: [ThinLTO] Allow multiple summary entries..

Alternatively (and likely preferably from a compile-time point of view to limit the list of import), we could keep a map of GUID->Summary and reuse it before trying to select a new callee.

Yep, that was my third option. I only hesitate because it seems like it might be better to select the prevailing copy when we can, but perhaps it doesn't matter much.

Jul 13 2017, 10:03 AM
mehdi_amini added a comment to D35081: [ThinLTO] Allow multiple summary entries..

Thanks, very clear :)

Jul 13 2017, 9:22 AM
mehdi_amini added inline comments to D33946: [InlineCost] Find identical loads in the callee.
Jul 13 2017, 9:07 AM
mehdi_amini added a comment to D35081: [ThinLTO] Allow multiple summary entries..

Teresa: can you describe a bit more how we end up importing two summaries for the same GUID? I would expect that after importing one, we don't even come to try to import another since we already have one.

Jul 13 2017, 9:04 AM

Jul 12 2017

mehdi_amini added inline comments to D33946: [InlineCost] Find identical loads in the callee.
Jul 12 2017, 8:14 PM
mehdi_amini added inline comments to D35334: ThinLTO Minimized Bitcode File Size Reduction.
Jul 12 2017, 6:15 PM
mehdi_amini added inline comments to D35334: ThinLTO Minimized Bitcode File Size Reduction.
Jul 12 2017, 5:48 PM
mehdi_amini added a comment to D35081: [ThinLTO] Allow multiple summary entries..

It's @yunlian, so if the issue you described above covers his failure, than that's great. @tejohnson do you have time to work on a fix soon? Otherwise I could give it a try, I would probably need a few pointers though, as I am not really familiar with ThinLTO.

Jul 12 2017, 5:29 PM
mehdi_amini added inline comments to D35334: ThinLTO Minimized Bitcode File Size Reduction.
Jul 12 2017, 5:25 PM
mehdi_amini added a comment to D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.

This LGTM. Mehdi, do you have any other concerns?

Jul 12 2017, 1:50 PM
mehdi_amini added inline comments to D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.
Jul 12 2017, 1:49 PM
mehdi_amini added inline comments to D35310: [PM] Use range-based for loops in LegacyPassManager.cpp (NFC)..
Jul 12 2017, 9:39 AM

Jul 11 2017

mehdi_amini accepted D21723: [RFC] Enhance synchscope representation.

With @sanjoy approval of langref, LGTM.

Jul 11 2017, 10:19 AM

Jul 10 2017

mehdi_amini accepted D35233: [WebAssembly] Fix use of cast vs dyn_cast.

LGTM, thanks!

Jul 10 2017, 6:31 PM
mehdi_amini added inline comments to D34345: [LLD][ELF] Extract temporary state used in assignAddresses().
Jul 10 2017, 4:21 PM
mehdi_amini added inline comments to D34345: [LLD][ELF] Extract temporary state used in assignAddresses().
Jul 10 2017, 4:19 PM
mehdi_amini added inline comments to D35029: [WebAssembly] Support weak defined symbols.
Jul 10 2017, 4:17 PM
mehdi_amini added inline comments to D33842: CodeGen: Fix address space of global variable.
Jul 10 2017, 4:14 PM
mehdi_amini added a comment to D35189: Add GUID-ValueID map and original name to ThinLTO summary..

The context and the motivation is *still* not clear to me. To begin why do we need the *names* when doing the thin link?

We need the GUID because it is needed when we build the combined index in thin link phase. We need ValueId is because the size of GUID is too large so that we do not want to write it everywhere, so we set a ValueId which is indexing from 0. And we should also set a GUID to ValueId map to record the mapping between them.
We need the original name because it is needed in FunctionImport. So we should pass is to thin link phase.

Jul 10 2017, 11:01 AM
mehdi_amini added a comment to D35189: Add GUID-ValueID map and original name to ThinLTO summary..
Jul 10 2017, 9:16 AM

Jul 9 2017

mehdi_amini added a comment to D35096: Increase the import-threshold for crtical functions..

Missing test showing the import happening on critical and not without. Right now the test you provided only shows that the "critical" flag is encoded in the binary. The change in the function importer is not tested, unless I missed something.

Jul 9 2017, 7:54 PM
mehdi_amini added a comment to D35189: Add GUID-ValueID map and original name to ThinLTO summary..

The context and the motivation is *still* not clear to me. To begin why do we need the *names* when doing the thin link?

Jul 9 2017, 7:42 PM
mehdi_amini added a comment to D35148: Use DenseMap instead std::map for GVSummaryMapTy..

When we used an std::map originally it was because we needed the ordering. Have you checked that we don't iterate on any instance of this map?

From http://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h it doesn't appear that iteration order is an issue for DenseMap.

Jul 9 2017, 7:35 PM
mehdi_amini added a comment to D35128: Add GUID-ValueID map and original name to ThinLTO summary..

As Teresa mentioned, please abandon this revision and recreate a new one.

Jul 9 2017, 11:21 AM
mehdi_amini updated the diff for D34992: Emit static constexpr member as available_externally definition.

Fix issues around mutable fields and regression on "internal", add more testing.

Jul 9 2017, 11:15 AM
mehdi_amini added a comment to D35148: Use DenseMap instead std::map for GVSummaryMapTy..

When we used an std::map originally it was because we needed the ordering. Have you checked that we don't iterate on any instance of this map?

Jul 9 2017, 11:02 AM
mehdi_amini added inline comments to D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.
Jul 9 2017, 6:14 AM
mehdi_amini added inline comments to D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.
Jul 9 2017, 6:13 AM

Jul 7 2017

mehdi_amini added a comment to D35128: Add GUID-ValueID map and original name to ThinLTO summary..

Please add the motivation/context for this patch, and a test. Thanks!

Jul 7 2017, 9:52 AM

Jul 6 2017

mehdi_amini added a comment to D35043: [ADT] Enable reverse iteration for DenseMap.

@davidxl: while not a bug, it is still annoying from an internal design point of view to tie our behavior to the hash function used in StringMap. Conceptually they are not supposed to be ordered!

Jul 6 2017, 11:53 AM
mehdi_amini added a comment to D35043: [ADT] Enable reverse iteration for DenseMap.

I had a similar issue when I tried to change the StringMap hash function in LLVM and it broke a bunch of test because LLVM's behavior changes depending on the order of iteration there (this is deterministic but the hash function is part of the inputs to consider for determinism basically...)

Jul 6 2017, 12:25 AM

Jul 5 2017

mehdi_amini added inline comments to D34992: Emit static constexpr member as available_externally definition.
Jul 5 2017, 4:36 PM
mehdi_amini added inline comments to D34992: Emit static constexpr member as available_externally definition.
Jul 5 2017, 4:14 PM
mehdi_amini accepted D33109: Enhance synchscope representation (clang).

Make sure your commit message correctly reference the LLVM change.

Jul 5 2017, 12:48 PM
mehdi_amini added inline comments to D34992: Emit static constexpr member as available_externally definition.
Jul 5 2017, 11:48 AM
mehdi_amini added a comment to D34197: Added Dockerfiles to build clang from sources..

If we start with an "easy to build" full image, it should be the same as the official packages: http://releases.llvm.org ; this is *not* what was proposed here.

I don't understand yet why would we need something that is the same as the official packages. I'd expect more something like an experience a distro would give you.

Jul 5 2017, 8:25 AM

Jul 4 2017

mehdi_amini created D34992: Emit static constexpr member as available_externally definition.
Jul 4 2017, 3:33 PM
mehdi_amini added a comment to D34197: Added Dockerfiles to build clang from sources..

Thanks! LGTM.

I'll be happy to iterate separately on providing an easy way to build a boostrap of clang (that would be easy to use with or without Docker)!

As a data point - I just tried to build the docker image and without Ilya's help I wouldn't have known to look at the docs (usually docker build envs for projects just work out of the box to give me a full image; I don't know whether you have a different experience). We'll improve comments on the script itself, but as a user I'd really like to have something that I can run that will Just Work :)

Jul 4 2017, 11:04 AM

Jun 30 2017

mehdi_amini added a comment to D34910: Make LLVM_TARGETS_TO_BUILD=all build all targets.

IIRC the intention was that in-tree work should not be burdened by experimental targets, up to and including breaking them. E.g. an in-tree API migration wouldn't have to update experimental targets. So in that sense LLVM_TARGETS_TO_BUILD=all is working as intended I think.

Jun 30 2017, 4:40 PM

Jun 29 2017

mehdi_amini accepted D34197: Added Dockerfiles to build clang from sources..
Jun 29 2017, 10:31 AM
mehdi_amini added a comment to D34197: Added Dockerfiles to build clang from sources..

I'll be happy to iterate separately on providing an easy way to build a boostrap of clang (that would be easy to use with or without Docker)!

Jun 29 2017, 10:30 AM
mehdi_amini added inline comments to D34197: Added Dockerfiles to build clang from sources..
Jun 29 2017, 10:24 AM
mehdi_amini added inline comments to D34197: Added Dockerfiles to build clang from sources..
Jun 29 2017, 8:57 AM
mehdi_amini added inline comments to D34197: Added Dockerfiles to build clang from sources..
Jun 29 2017, 8:47 AM

Jun 28 2017

mehdi_amini added inline comments to D34197: Added Dockerfiles to build clang from sources..
Jun 28 2017, 12:52 PM

Jun 27 2017

mehdi_amini added a comment to D24644: Pass -ffunction-sections/-fdata-sections along to gold-plugin.

I'd like to fix PR22999 and was wondering if you think adding a function-section attribute to the IR would be a viable solution?

When doing LTO, we could add the same function-section to each function in a module in the IRLinker. @mehdi_amini did you think something like that when suggesting using attributes?

Jun 27 2017, 9:08 AM
mehdi_amini added a comment to D34028: [Bitcode] Add thumb-mode to target-features in metadata loader..

(maybe it is already?)

Jun 27 2017, 8:10 AM
mehdi_amini added a comment to D34028: [Bitcode] Add thumb-mode to target-features in metadata loader..

Should this be documented in the LLVM LangRef? (or elsewhere?)
(Otherwise I'd expect some documentation on the `upgradeThumbMode``` function at least)

Jun 27 2017, 8:09 AM

Jun 26 2017

mehdi_amini added a comment to D33946: [InlineCost] Find identical loads in the callee.

The description says "The motivation example is a fully unrolled 3x3 matrix multiplication. It loads every data in matrix a and b three times because of the Stores between them", but from reading the code I don't see how this case is handled since it seems that you bail as soon as you see a store (or a call)? What did I miss? Is the description still up-to-date?

Jun 26 2017, 11:05 PM
mehdi_amini added a comment to D34653: Reduce the size of ThinLTO summary file.

Also, please upload patches with full context (git diff -U999999)

Jun 26 2017, 5:15 PM
mehdi_amini added a comment to D34653: Reduce the size of ThinLTO summary file.

This seems quite intrusive and adding a quite some complexity inline (and increasing the invariant surface for BitcodeWriterBase apparently).
I'm not convinced that this is the right approach at this point: we're really stretching the `WriteBitcodeToFile``` API quite far.

Jun 26 2017, 5:15 PM
mehdi_amini added inline comments to D34028: [Bitcode] Add thumb-mode to target-features in metadata loader..
Jun 26 2017, 9:17 AM
mehdi_amini added inline comments to D34450: bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals (release_40).
Jun 26 2017, 9:10 AM

Jun 23 2017

mehdi_amini committed rL306134: Fix typo: using && instead of & when evaluating a mask.
Fix typo: using && instead of & when evaluating a mask
Jun 23 2017, 11:21 AM
mehdi_amini closed D34550: Fix typo: using && instead of & when evaluating a mask by committing rL306134: Fix typo: using && instead of & when evaluating a mask.
Jun 23 2017, 11:20 AM

Jun 22 2017

mehdi_amini added inline comments to D34546: docs: Add documentation for the ThinLTO cache pruning policy string..
Jun 22 2017, 10:25 PM
mehdi_amini created D34550: Fix typo: using && instead of & when evaluating a mask.
Jun 22 2017, 10:19 PM