mehdi_amini (Mehdi AMINI)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 30 2013, 5:34 PM (233 w, 21 h)

Recent Activity

Wed, Oct 11

mehdi_amini added inline comments to D38808: [CMake] Improve logic for enabling LTO on LLVM itself.
Wed, Oct 11, 8:37 PM

Mon, Oct 2

mehdi_amini 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?

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

Mehdi, would you feel comfortable with this as-is?

Mon, Oct 2, 5:49 PM

Sun, Oct 1

mehdi_amini added inline comments to D38154: [PassManager] Run global opts after the inliner.
Sun, Oct 1, 10:58 PM
mehdi_amini added inline comments to D38154: [PassManager] Run global opts after the inliner.
Sun, Oct 1, 10:48 PM
mehdi_amini added inline comments to D38154: [PassManager] Run global opts after the inliner.
Sun, Oct 1, 10:29 PM

Thu, Sep 28

mehdi_amini added inline comments to D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds..
Thu, Sep 28, 10:54 AM
mehdi_amini added inline comments to D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds..
Thu, Sep 28, 10:23 AM
mehdi_amini added a comment to D38298: A logic to copy LLVM licences into docker images..

And it would be nice to have this change right now as a quick workaround.

Thu, Sep 28, 9:42 AM
mehdi_amini added a comment to D38298: A logic to copy LLVM licences into docker images..

"make install" is not primarily a distribution mechanism, docker is (most folks upload their docker images to public registries without thinking much about this).

I'm not sure why "primarily a distribution mechanism matters? How is one suppose to distribute?

docker push to a public registry?

Thu, Sep 28, 8:34 AM
mehdi_amini added a comment to D38298: A logic to copy LLVM licences into docker images..

"make install" is not primarily a distribution mechanism, docker is (most folks upload their docker images to public registries without thinking much about this).

Thu, Sep 28, 1:54 AM

Wed, Sep 27

mehdi_amini added a comment to D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds..

Ideally, I think all the headers would hide the method behind #ifdef LLVM_ENABLE_DUMP , so that it wouldn't be a linker failure but a compile time failure!

Wed, Sep 27, 3:01 PM
mehdi_amini added a comment to D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds..

Thanks for fixing this :)

Wed, Sep 27, 2:59 PM
mehdi_amini added a comment to D38298: A logic to copy LLVM licences into docker images..

Can you elaborate why this script is desirable/needed?

Sure, sorry for leaving out the context.

Including licences is technically required in any binary distribution of LLVM, including ones inside Docker images.
Copying the licenses could be made optional, but always putting the licenses into Docker images should not hurt either.

Wed, Sep 27, 1:49 PM

Tue, Sep 26

mehdi_amini added a comment to D38298: A logic to copy LLVM licences into docker images..

Can you elaborate why this script is desirable/needed?

Tue, Sep 26, 4:19 PM

Mon, Sep 18

mehdi_amini added inline comments to D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.
Mon, Sep 18, 5:05 PM

Sep 17 2017

mehdi_amini accepted D37961: [ThinLTO] Avoid archive member collisions with old API.
Sep 17 2017, 10:27 AM
mehdi_amini added a comment to D37961: [ThinLTO] Avoid archive member collisions with old API.

Did you check ld64 in Xcode 9? I think ld64 was fixed to append the suffix as well.

Sep 17 2017, 10:18 AM

Sep 4 2017

mehdi_amini committed rL312512: Emit static constexpr member as available_externally definition.
Emit static constexpr member as available_externally definition
Sep 4 2017, 8:59 PM

Sep 1 2017

mehdi_amini added a comment to D37410: LTO: Try to open cache files before renaming them..

LGTM

Sep 1 2017, 10:40 PM

Aug 31 2017

mehdi_amini added a comment to D37370: ModuleSummaryAnalysis: Correctly handle refs from function inline asm to module inline asm..

LGTM otherwise.

Aug 31 2017, 10:34 PM
mehdi_amini added inline comments to D37370: ModuleSummaryAnalysis: Correctly handle refs from function inline asm to module inline asm..
Aug 31 2017, 10:34 PM

Aug 28 2017

mehdi_amini added a comment to D37213: Changed Dockerfiles to install LLVM into /usr/local.

I don't mind where you install it, but we can also assume that someone who use this Dockerfile intends to use clang, so why not adding it to to update-alternatives as well, and even making it default for /usr/bin/c++ and /usr/bin/cc?

Aug 28 2017, 9:21 AM

Aug 27 2017

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

I'd like to also see a testcase for the situation where we trigger the emission of a declaration with an available_externally definition and then find we need to promote it to a "full" definition:

Aug 27 2017, 1:51 PM
mehdi_amini committed rL311858: CMake: only try to find the Z3 package when `CLANG_ANALYZER_BUILD_Z3` is ON.
CMake: only try to find the Z3 package when `CLANG_ANALYZER_BUILD_Z3` is ON
Aug 27 2017, 1:25 PM
mehdi_amini committed rL311857: Emit static constexpr member as available_externally definition.
Emit static constexpr member as available_externally definition
Aug 27 2017, 1:25 PM
mehdi_amini closed D34992: Emit static constexpr member as available_externally definition by committing rL311857: Emit static constexpr member as available_externally definition.
Aug 27 2017, 1:25 PM

Aug 25 2017

mehdi_amini added a comment to D37099: Added optional validation of svn sources to Dockerfiles..

Would this be all obsolete with git repos?

I would think so.

Also all the python code seems complicated to me for what is basically find | grep -v '/\.git/' | grep -v '/\.svn/' | LC_ALL=C sort | tar -cf - -T - --no-recursion | sha1sum, can you elaborate why this is needed?

That's roughly what I started with, but's it's more complicated if you want to compute separate checksums for projects inside single tree source checkout (i.e. llvm, llvm/tool/clang, llvm/projects/lldb, etc.).

Aug 25 2017, 11:52 AM

Aug 24 2017

mehdi_amini accepted D37098: Use temporary directory when building docker image..
Aug 24 2017, 10:15 PM
mehdi_amini added a comment to D37099: Added optional validation of svn sources to Dockerfiles..

Would this be all obsolete with git repos?

Aug 24 2017, 10:01 PM

Aug 20 2017

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

Aug 17 2017

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

Bi-weekly ping! (@rsmith)

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

LGTM

Aug 17 2017, 8:17 AM

Aug 14 2017

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)

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

Aug 9 2017

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)

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

Aug 8 2017

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

LGTM.

Aug 8 2017, 10:02 PM · lld

Aug 7 2017

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

Aug 5 2017

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

Aug 4 2017

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

Address Richard's comment

Aug 4 2017, 2:17 AM

Aug 3 2017

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

I'll leave the final approval to Teresa.

Aug 3 2017, 4:48 PM
mehdi_amini added inline comments to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.
Aug 3 2017, 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.

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

Ping again @rsmith (or anyone else)

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

Aug 2 2017

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.

Aug 2 2017, 11:12 AM
mehdi_amini added inline comments to D36233: [ThinLTO] Add FunctionAttrs to ThinLTO index.
Aug 2 2017, 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?

Aug 2 2017, 11:05 AM

Jul 31 2017

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...

Jul 31 2017, 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).

Jul 31 2017, 10:11 PM

Jul 30 2017

mehdi_amini added inline comments to D32471: IR: Use pointers instead of GUIDs to represent edges in the module summary. NFCI..
Jul 30 2017, 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)?

Jul 30 2017, 10:36 AM

Jul 26 2017

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.

Jul 26 2017, 6:21 PM

Jul 25 2017

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.

Jul 25 2017, 9:03 PM

Jul 24 2017

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

Weekly ping! (@rsmith)

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

Here is the current output:

Jul 24 2017, 8:53 AM
mehdi_amini added a reviewer for D33900: Print registered targets in clang's version information: hans.
Jul 24 2017, 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