mehdi_amini (Mehdi AMINI)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 30 2013, 5:34 PM (241 w, 14 h)

Recent Activity

Fri, Dec 8

mehdi_amini accepted D40970: [ThinLTO] Remove unused (?) code from thinLTOInternalizeModule.

I wrote this code in April 2016 ( http://reviews.llvm.org/D19103 ), in the very first approach to internalization, which was inferred in the backend and wasn't bullet proof (especially with respect to assembly by the way).
Since then, we moved all decision in the thin-link and this code is indeed (hopefully) dead.

Fri, Dec 8, 9:16 AM
mehdi_amini added inline comments to D40970: [ThinLTO] Remove unused (?) code from thinLTOInternalizeModule.
Fri, Dec 8, 9:10 AM

Tue, Nov 14

mehdi_amini accepted D40005: NFC Remove default argument of DataLayout::getPointerABIAlignment.

LGTM.
Did you check clang?

Tue, Nov 14, 8:45 PM
mehdi_amini accepted D40041: Use TempFile in lto caching.

LGTM

Tue, Nov 14, 8:43 PM

Mon, Nov 13

mehdi_amini added a comment to D39865: Use default member initialization where possible.

Was this done using clang-tidy (or similar tool) or manually?

Mon, Nov 13, 5:44 PM

Nov 7 2017

mehdi_amini added a comment to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..

Oh ok, this is different than what thought the issue was. There isn't a correctness problem with this - the reference edge ensures correctness. It sounds like the only correctness issue then was due to the ld64 issue. (I thought there was also an issue with getting @f promoted when @ext was imported.) There isn't really a correctness issue with having both a reference and call edge though. Ideally we would have just the call edge and no reference edge, but that goes back to the earlier suggestion you had tried (changing the call site to look through pointer casts which had other issues). So I think this is fine. Sorry for the confusion.

Nov 7 2017, 8:13 PM

Nov 6 2017

mehdi_amini added a reviewer for D39373: Reorder Value.def to optimize code size: pcc.
Nov 6 2017, 10:44 AM
mehdi_amini added a reviewer for D39373: Reorder Value.def to optimize code size: rnk.
Nov 6 2017, 10:44 AM

Nov 3 2017

mehdi_amini added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Nov 3 2017, 6:02 PM
mehdi_amini committed rL317391: Fix CMake definitions of tsan runtime to make it installed by "install-compiler….
Fix CMake definitions of tsan runtime to make it installed by "install-compiler…
Nov 3 2017, 5:07 PM
mehdi_amini closed D39621: Fix CMake definitions of tsan runtime to make it installed by "install-compiler-rt" by committing rL317391: Fix CMake definitions of tsan runtime to make it installed by "install-compiler….
Nov 3 2017, 5:07 PM
mehdi_amini created D39621: Fix CMake definitions of tsan runtime to make it installed by "install-compiler-rt".
Nov 3 2017, 3:53 PM

Nov 2 2017

mehdi_amini added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Nov 2 2017, 6:05 PM

Oct 31 2017

mehdi_amini added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Oct 31 2017, 8:37 PM

Oct 27 2017

mehdi_amini added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Oct 27 2017, 1:23 PM

Oct 26 2017

mehdi_amini added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Oct 26 2017, 8:24 PM

Oct 11 2017

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

Oct 2 2017

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?

Oct 2 2017, 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?

Oct 2 2017, 5:49 PM

Oct 1 2017

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

Sep 28 2017

mehdi_amini added inline comments to D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds..
Sep 28 2017, 10:54 AM
mehdi_amini added inline comments to D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds..
Sep 28 2017, 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.

Sep 28 2017, 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?

Sep 28 2017, 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).

Sep 28 2017, 1:54 AM

Sep 27 2017

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!

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

Thanks for fixing this :)

Sep 27 2017, 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.

Sep 27 2017, 1:49 PM

Sep 26 2017

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?

Sep 26 2017, 4:19 PM

Sep 18 2017

mehdi_amini added inline comments to D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.
Sep 18 2017, 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