Page MenuHomePhabricator

mehdi_amini (Mehdi AMINI)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 30 2013, 5:34 PM (293 w, 1 d)

Recent Activity

Mon, Nov 26

mehdi_amini added a comment to D54877: Update coding guidelines regarding use of `auto`.

at the end of the day, comments that arise in code review should take precedence over what the style guide does or doesn't say when it concerns readability, since it's an actual account of someone stating that they do or don't find something readable, and that's what we're trying to optimize for.

Mon, Nov 26, 8:35 PM
mehdi_amini added inline comments to D54877: Update coding guidelines regarding use of `auto`.
Mon, Nov 26, 11:59 AM
mehdi_amini updated subscribers of D35035: [InstCombine] Prevent memcpy generation for small data size.

That case (and a couple of similar tests that I tried) are handled by -sroa, so they probably never made it to instcombine in the 1st place. I don't know anything about SROA, but hopefully, it's making that transform using some principled logic. :)

Mon, Nov 26, 9:50 AM

Sun, Nov 25

mehdi_amini added a comment to D54887: WIP: hack to use DL for promoting to load/store.

(should fix PR39780 https://bugs.llvm.org/show_bug.cgi?id=39780 )

Sun, Nov 25, 10:42 PM
mehdi_amini updated the summary of D54888: Optimize IRGen for load/stores of bitfields.
Sun, Nov 25, 10:40 PM
mehdi_amini added a parent revision for D54888: Optimize IRGen for load/stores of bitfields: D54887: WIP: hack to use DL for promoting to load/store.
Sun, Nov 25, 10:34 PM
mehdi_amini created D54888: Optimize IRGen for load/stores of bitfields.
Sun, Nov 25, 10:34 PM
mehdi_amini added a child revision for D54887: WIP: hack to use DL for promoting to load/store: D54888: Optimize IRGen for load/stores of bitfields.
Sun, Nov 25, 10:34 PM

Oct 22 2018

mehdi_amini added a comment to D53414: Add instructions for migrating branches from one git repository to another..

If we go with this document moving forward, I would first purge it so that it reflects the current situation. Otherwise we could also just archive it and start a new one with the actual action plan.
I feel adding more stuff here will be a bit confusing moving forward.

Oct 22 2018, 4:26 PM

Sep 13 2018

mehdi_amini added inline comments to D52023: [ThinLTO]Allow setting of maximum cache size with 64-bit number, and provide C-interface function for large values.
Sep 13 2018, 8:49 AM

Sep 10 2018

mehdi_amini added a comment to D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal..

@kristina both bfd and lld are adding the .a extension when you pass a lib through -l. Similarly to -llibfoo won't work, -lfoo.a will lead the linker to look for libfoo.a.a

Sep 10 2018, 5:23 PM
mehdi_amini edited reviewers for D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal., added: mehdi_amini; removed: joker-eph-DISABLED.
Sep 10 2018, 5:20 PM

Aug 20 2018

mehdi_amini added inline comments to D43690: [ThinLTO] Keep available_externally symbols live.
Aug 20 2018, 10:14 AM
mehdi_amini added inline comments to D43690: [ThinLTO] Keep available_externally symbols live.
Aug 20 2018, 10:12 AM

Aug 16 2018

mehdi_amini added inline comments to D43690: [ThinLTO] Keep available_externally symbols live.
Aug 16 2018, 9:21 PM
mehdi_amini accepted D50882: [ThinLTO] Correct documentation on default number of threads.

LGTM

Aug 16 2018, 9:14 PM

Aug 8 2018

mehdi_amini accepted D50476: [NFC] ConstantMerge: don't insert when find should be used.

LGTM

Aug 8 2018, 4:18 PM

Jul 31 2018

mehdi_amini added a comment to D49771: CodeGen: use non-zero memset when possible for automatic variables.

I'm worried, however, about generating a bunch more code than needed from clang in the hopes that the compiler will clean it up later.

Jul 31 2018, 5:10 PM
mehdi_amini added a comment to D49771: CodeGen: use non-zero memset when possible for automatic variables.
In D49771#1183380, @jfb wrote:

I'm curious: isn't the kind of optimization we should expect LLVM to provide?

Maybe? It seems obvious to do here since we know we'll probably want to be doing it, and I have another patch I'm working on which will make it that much more obviously useful to have here. The middle-end can definitely figure it out but it just seems like more work, later, so in the meantime we'd be looking at more stuff.

Jul 31 2018, 3:58 PM

Jul 30 2018

mehdi_amini added a comment to D49771: CodeGen: use non-zero memset when possible for automatic variables.

I'm curious: isn't the kind of optimization we should expect LLVM to provide?

Jul 30 2018, 1:55 PM

Jul 25 2018

mehdi_amini added a comment to D49777: [LTO] Don't internalize declarations.
but declarations cannot be internal.
Jul 25 2018, 2:00 PM

Jul 10 2018

mehdi_amini accepted D48783: [ThinLTO] Use std::map to get determistic imports files.

LGTM.

Jul 10 2018, 12:42 PM
mehdi_amini added a comment to D48783: [ThinLTO] Use std::map to get determistic imports files.

std::map is usually fairly slow, have you considered sorting at the time you write to the file instead? That would add a slow step when writing the files but wouldn't affect any in-memory operation.

Jul 10 2018, 10:50 AM

Feb 19 2018

mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

Note: we need buy-in from LLVM 6.0 release manager (@hans ?) that this will be cherry-picked there before landing this in trunk.

This seems fine for 6.0 (it needs to land soon though), but why does it need to be cherry-picked to the branch before landing on trunk?

Feb 19 2018, 11:37 AM

Feb 16 2018

mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

Feb 16 2018, 11:56 AM
mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

@hans : this needs to go in LLVM 6.0, is it OK?

Feb 16 2018, 8:21 AM
mehdi_amini added a reviewer for D43253: bitcode support change for fast flags compatibility: hans.
Feb 16 2018, 8:21 AM

Feb 15 2018

mehdi_amini updated subscribers of D43253: bitcode support change for fast flags compatibility.

Strongly disagree. The new test's purpose is to provide functional testing on the new flags in the new format. Sanjay, now would be a good time to chime in.

Feb 15 2018, 1:42 PM
mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

Case in point:

./bin/llvm-dis < /Users/michael_c_berg/llvm/llvm/test/Bitcode/compatibility-3.6.ll.bc | ./bin/FileCheck ../llvm/test/Bitcode/compatibility-3.6.ll
../llvm/test/Bitcode/compatibility-3.6.ll:617:11: error: expected string not found in input

; CHECK: %f.fast = fadd reassoc nnan ninf nsz arcp float %op1, %op2
         ^

<stdin>:445:2: note: scanning from here

%f.fast = fadd fast float %op1, %op2

in this case we are using my current patch with module version at 2 in the key .bc file, however we need the mapping as:

reassoc
nnan
ninf
nsz
arch
contract
afn

the new format writes reassoc in position 0 and aliases to UnsafeAlgebra incorrectly in this case, causing us to map fast, when less than fast was specified in the IR.
This will still be the case with the other format.

^
Feb 15 2018, 1:31 PM
mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

So the compatibility .bc files in check test have usages of the current flag layout with reassoc, afn and the new layout, as has been the case since last Nov. These tests are not old fast format but sub flags tests in the new format, and require the new mapping.

Feb 15 2018, 1:11 PM
mehdi_amini added a comment to D43348: Make FastMathFlags bitcode serialization to match LLVM 5.0 layout.

@tstellar YES.

Feb 15 2018, 1:00 PM
mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

For the 6 bitcode files, they are special context, as they require the current fast sub flags format for part of their use, so we have to bring them up into the current context as they require the use of the new flags selectively with the bumped version number.

Can you elaborate what you mean by "they require the current fast sub flags format for part of their use"?

Feb 15 2018, 1:00 PM
mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

I quickly put a patch to change the encoding, may be easier to discuss with code at hand: https://reviews.llvm.org/D43348
(I didn't try to build it, just a quick draft)

Feb 15 2018, 12:11 PM
mehdi_amini created D43348: Make FastMathFlags bitcode serialization to match LLVM 5.0 layout.
Feb 15 2018, 12:11 PM
mehdi_amini added inline comments to D43253: bitcode support change for fast flags compatibility.
Feb 15 2018, 11:19 AM
mehdi_amini added inline comments to D43253: bitcode support change for fast flags compatibility.
Feb 15 2018, 10:16 AM

Feb 14 2018

mehdi_amini added inline comments to D43253: bitcode support change for fast flags compatibility.
Feb 14 2018, 8:38 PM

Feb 13 2018

mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

Actually if we're making this change, we consider that bitcode generated between the introduction of the new fast-math flags and now isn't supported anymore.
If so then we could instead of bumping a version, change the encoding of the FMF record (or create a new record type), so that the old encoding is upgraded but not the new one.
That would seem to me to be more in line with the usual handling of such situation in the bitcode.

Feb 13 2018, 9:39 PM
mehdi_amini added inline comments to D43253: bitcode support change for fast flags compatibility.
Feb 13 2018, 9:36 PM
mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

For the 6 bitcode files, they are special context, as they require the current fast sub flags format for part of their use, so we have to bring them up into the current context as they require the use of the new flags selectively with the bumped version number.

Feb 13 2018, 9:16 PM
mehdi_amini added inline comments to D43253: bitcode support change for fast flags compatibility.
Feb 13 2018, 1:06 PM
mehdi_amini added a comment to D43253: bitcode support change for fast flags compatibility.

Uploaded above are the 6 bitcode files we will need to replace for check to run successfully as these files had the old version number in them.

Feb 13 2018, 1:02 PM

Feb 6 2018

mehdi_amini updated subscribers of D42267: [ThinLTO] Allow 0 to be a valid value for pruning interval for C LTO API..
Feb 6 2018, 1:04 PM

Jan 13 2018

mehdi_amini added a comment to D41297: [ThinLTO] Implement summary visualizer.

At which point of the API is this guarantee?

  1. ThinLTO.ModuleMap stores BitcodeModule objects and uses them during entire thin link phase (until backend threads are launched)
  2. BitcodeModule contains pointers to BC data, so this data should be actual for module to be parsed)
  3. Module summary is exported to .dot in CombinedIndexHook which is invoked in the beginning of thin link phase

None of these seems like API guarantee to me, it sounds like "this is what actually happens in practice today in this particular workflow".

It seems it must be a guarantee of the LTO API, since ThinLTO.ModuleMap holds the BitcodeModule objects and accesses their buffers through and after the thin link.

Jan 13 2018, 10:01 AM

Jan 12 2018

mehdi_amini added a comment to D41297: [ThinLTO] Implement summary visualizer.

At which point of the API is this guarantee?

  1. ThinLTO.ModuleMap stores BitcodeModule objects and uses them during entire thin link phase (until backend threads are launched)
  2. BitcodeModule contains pointers to BC data, so this data should be actual for module to be parsed)
  3. Module summary is exported to .dot in CombinedIndexHook which is invoked in the beginning of thin link phase
Jan 12 2018, 11:03 AM
mehdi_amini added inline comments to D41297: [ThinLTO] Implement summary visualizer.
Jan 12 2018, 8:58 AM

Jan 10 2018

mehdi_amini accepted D41465: [LTO] Support filtering by hotness threshold.

LGTM

Jan 10 2018, 6:22 AM

Jan 9 2018

mehdi_amini accepted D41624: Fix crash when linking metadata with ODR type uniquing.

The test still seems complex to me (do you need all the IR types? The code in the functions? And all these metadatas and metadata fields?

Jan 9 2018, 9:53 AM

Dec 29 2017

mehdi_amini added inline comments to D41624: Fix crash when linking metadata with ODR type uniquing.
Dec 29 2017, 1:00 AM

Dec 21 2017

mehdi_amini added inline comments to D41296: Limit size of non-GlobalValue name.
Dec 21 2017, 1:39 PM
mehdi_amini added a comment to D41296: Limit size of non-GlobalValue name.

Why don't we just cap all names at some point (and then just start adding numbers as we generally do to break degeneracies). It seems like, otherwise, we'll end up with these kinds of fixes in many places. Fixing this in one common place seems better.

I thought of this, very happy you suggest we go that way. In a release build, we could activate that by default and keep the names in debug build.

Dec 21 2017, 9:26 AM
mehdi_amini added inline comments to D41474: Fix a crash in lazy loading of Metadata in ThinLTO.
Dec 21 2017, 9:20 AM

Dec 18 2017

mehdi_amini added inline comments to D41279: [ThinLTO][C-API] Correct api comments.
Dec 18 2017, 9:49 AM
mehdi_amini added a comment to D41297: [ThinLTO] Implement summary visualizer.

@mehdi_amini I've updated diff with one which no longer requires post-processing at the cost of having extra StringRef in GlobalValueSummaryInfo

add notation about the importing decision, promotion, dead-elimination, ... on every edge/nodes.

It this supposed to be done as comments in DOT file?

Dec 18 2017, 9:03 AM

Dec 15 2017

mehdi_amini added a comment to D41267: [LTO] Make processing of combined module more consistent.

I agree that implicit contract about streams order is not nice here, and we should consider to make it explicit.

Dec 15 2017, 9:09 PM
mehdi_amini added a comment to D41267: [LTO] Make processing of combined module more consistent.

However small changes in input, could trigger combined module and shuffle outputs making life of llvm::LTO harder.

Sorry but this is still loaded in assumption about the client, which seems dubious in general to me.

At this point I am trying to solve problem is the gold plugin, however I'd expect this could be helpful for other clients as well.
Anyway why benefit of particular client without regressing behavior for the rest is a problem? Especially considering that the patch simplify implementation.

Dec 15 2017, 6:53 PM
mehdi_amini added a comment to D41267: [LTO] Make processing of combined module more consistent.

However small changes in input, could trigger combined module and shuffle outputs making life of llvm::LTO harder.

Dec 15 2017, 1:24 PM
mehdi_amini added inline comments to D41279: [ThinLTO][C-API] Correct api comments.
Dec 15 2017, 9:46 AM
mehdi_amini added a comment to D41297: [ThinLTO] Implement summary visualizer.

I suspect it should be possible to do this after the ThinLink and add notation about the importing decision, promotion, dead-elimination, ... on every edge/nodes.

Dec 15 2017, 9:44 AM
mehdi_amini added a comment to D41297: [ThinLTO] Implement summary visualizer.

Neat!

Dec 15 2017, 9:43 AM
mehdi_amini added a comment to D41274: [LTO] NFC: remove unique_ptr from some LTO::RegularLTOState members.

This patch doesn't seem motivated, the only effect I can see is that a ThinLTO link that don't include any LTO module would create a module while it was lazily created only if needed before.

Dec 15 2017, 12:06 AM

Dec 14 2017

mehdi_amini added a comment to D41267: [LTO] Make processing of combined module more consistent.

The commit message does not make much sense to me in the context of this C++ API: there is a non-negligible layer between the build and the build system rules and the LTO implementation, i.e. the linker.
Implementing a contract that makes it convenient for your build system in the implementation of the LTO "engine" behind this API looks like leaking abstractions to me.

Dec 14 2017, 11:09 PM

Dec 8 2017

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.

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

Nov 14 2017

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

LGTM.
Did you check clang?

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

LGTM

Nov 14 2017, 8:43 PM

Nov 13 2017

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

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

Nov 13 2017, 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