mehdi_amini (Mehdi AMINI)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Jun 23

mehdi_amini committed rL306134: Fix typo: using && instead of & when evaluating a mask.
Fix typo: using && instead of & when evaluating a mask
Fri, Jun 23, 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.
Fri, Jun 23, 11:20 AM

Thu, Jun 22

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

LGTM.

Thu, Jun 22, 10:13 PM
mehdi_amini added inline comments to D34546: docs: Add documentation for the ThinLTO cache pruning policy string..
Thu, Jun 22, 10:08 PM

Tue, Jun 20

mehdi_amini added inline comments to D34063: [ThinLTO] YAML traits for module summaries.
Tue, Jun 20, 11:25 AM

Mon, Jun 19

mehdi_amini added a comment to D34197: Added Dockerfiles to build clang from sources..

Feel free to advocate for it, but except that stating an opinion above I'm missing you arguments.

For a newcomer or even a contributor, not already familiar with LLVM's build system, it's not at all obvious which arguments one must pass to cmake to make build work for him.

Mon, Jun 19, 11:00 AM
mehdi_amini accepted D33381: Avoid constructing GlobalExtensions only to find out it is empty..

LGTM with an inline comment

Mon, Jun 19, 8:56 AM
mehdi_amini added a comment to D34197: Added Dockerfiles to build clang from sources..

I would advocate for an easier interface to the build script, even if it would add some maintenance costs (that said, I'm ready to maintain it).
I think having a script, accepting parameters like 'make a 2-stage bootstrap and install lld+clang)', is a better interface than 'checkout projects cfe, lld, pass arguments "-X -Y -Z" to cmake'. Keeping an interface to the build script simple (while also not letting the maintenance costs blow up) should be a priority here, IMO.

Mon, Jun 19, 8:44 AM

Fri, Jun 16

mehdi_amini requested changes to D34197: Added Dockerfiles to build clang from sources..

I'm interested into seeing this landing! However I also have concerns right now:

Fri, Jun 16, 10:43 PM

Thu, Jun 15

mehdi_amini added a comment to D34268: [clang] Fix format specifiers fixits for nested macros.

Thanks for fixing! (I'm still not the best qualified to review this)

Thu, Jun 15, 8:30 PM

Wed, Jun 14

mehdi_amini added a comment to D33515: Force RegisterStandardPasses to construct std::function in the IPO library..

(If @efriedma thinks that the fix is the right one for the provided explanation that's fine with me)

Wed, Jun 14, 10:57 PM
mehdi_amini added a comment to D34156: [LTO] Enable module summary emission by default for regular LTO.

OK! Thanks both :)

Wed, Jun 14, 4:25 PM
mehdi_amini added a comment to D33515: Force RegisterStandardPasses to construct std::function in the IPO library..

If you need examples about what I'm expecting to read from a commit message (same applies to comment in the code though), you may look at these commit messages:

Wed, Jun 14, 4:23 PM
mehdi_amini added a comment to D33515: Force RegisterStandardPasses to construct std::function in the IPO library..

From the summary:

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

What would you like either the comment or the summary it to say?

Wed, Jun 14, 4:17 PM
mehdi_amini added inline comments to D34063: [ThinLTO] YAML traits for module summaries.
Wed, Jun 14, 2:35 PM
mehdi_amini added a comment to D34156: [LTO] Enable module summary emission by default for regular LTO.
  • Change patch to always emit a module summary for regular LTO

    I don't see any real downside of having a summary given the marginal time and space overhead it takes to construct and save it.
Wed, Jun 14, 2:30 PM
mehdi_amini added a comment to D33515: Force RegisterStandardPasses to construct std::function in the IPO library..

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

Don't understand this.
Is the comment really confusing/unhelpful?

Wed, Jun 14, 2:28 PM
mehdi_amini added a comment to D20217: Add direct control of whether or not a symbol is preemtable at runtime.

Missing LangRef update.

Wed, Jun 14, 2:25 PM

Tue, Jun 13

mehdi_amini added a comment to D33976: [clang] Fix format specifiers fixits.

@mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an example / test case would be helpful, that looks like a separate issue.
Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb could look at this diff (which, besides the fix, adds tests)

I'll CC you on the bug when I get time to reduce the test case and file the bug.

Tue, Jun 13, 6:02 PM
mehdi_amini added inline comments to D33515: Force RegisterStandardPasses to construct std::function in the IPO library..
Tue, Jun 13, 10:55 AM

Sun, Jun 11

mehdi_amini accepted D34086: Fix -print-after-all banner.

LGTM.

Sun, Jun 11, 10:22 AM

Sat, Jun 10

mehdi_amini added inline comments to D34063: [ThinLTO] YAML traits for module summaries.
Sat, Jun 10, 9:49 AM

Fri, Jun 9

mehdi_amini added inline comments to D34063: [ThinLTO] YAML traits for module summaries.
Fri, Jun 9, 7:12 PM
mehdi_amini added inline comments to D34063: [ThinLTO] YAML traits for module summaries.
Fri, Jun 9, 1:37 PM
mehdi_amini added a reviewer for D34055: Be more strict when checking the -flto option value: tejohnson.
Fri, Jun 9, 9:07 AM

Thu, Jun 8

mehdi_amini added a comment to D33976: [clang] Fix format specifiers fixits.

@mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an example / test case would be helpful, that looks like a separate issue.
Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb could look at this diff (which, besides the fix, adds tests)

Thu, Jun 8, 3:35 PM
mehdi_amini added a comment to D33125: Introduce isoneof<T0, T1, ...> as an extension of isa<T>.

I'm fine either way with this patch, and @chandlerc didn't answer my last comment.

Thu, Jun 8, 2:02 PM

Tue, Jun 6

mehdi_amini added inline comments to D31349: IR: Replace the "Linker Options" module flag with "llvm.linker.options" named metadata..
Tue, Jun 6, 10:31 PM
mehdi_amini added a comment to D33976: [clang] Fix format specifiers fixits.

Interestingly I got the opposite issue recently: calling through a macro with a single format specifier was *adding* new specific in the fixit in some conditions.

Tue, Jun 6, 10:26 PM
mehdi_amini added reviewers for D33976: [clang] Fix format specifiers fixits: ahatanak, arphaman.
Tue, Jun 6, 10:25 PM
mehdi_amini accepted D33900: Print registered targets in clang's version information.

LGTM

Tue, Jun 6, 2:30 PM
mehdi_amini accepted D33899: Allow VersionPrinter to print to arbitrary raw_ostreams.

LGTM, thanks.

Tue, Jun 6, 2:22 PM
mehdi_amini added inline comments to D33899: Allow VersionPrinter to print to arbitrary raw_ostreams.
Tue, Jun 6, 1:29 PM
mehdi_amini added inline comments to D33899: Allow VersionPrinter to print to arbitrary raw_ostreams.
Tue, Jun 6, 12:52 PM
mehdi_amini added a comment to D21723: [RFC] Enhance synchscope representation.

(Clarification the "code" part LGTM, so as soon as Sanjoy is OK with LangRef, that's fine with me)

Tue, Jun 6, 12:46 PM

Mon, Jun 5

mehdi_amini accepted D33924: [llvm] Remove double semicolons.

LGTM

Mon, Jun 5, 9:47 PM
mehdi_amini added inline comments to D33899: Allow VersionPrinter to print to arbitrary raw_ostreams.
Mon, Jun 5, 11:38 AM
mehdi_amini added a comment to D33886: Reduce the size of output with -print-before/after-all by avoid duplicated dump.

Could be useful, but that seems a bit intrusive right now (ad-hoc hooks in Module and PassManager)

Mon, Jun 5, 11:38 AM
mehdi_amini added a comment to D33513: [ThinLTO] Fix ThinLTO crash while destroying context.

Addressed comments. Formatted with clang-format, so that's why there are so many extra changes in the diff.

Mon, Jun 5, 8:38 AM · lld

Thu, Jun 1

mehdi_amini added inline comments to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.
Thu, Jun 1, 5:16 PM
mehdi_amini added a comment to D23615: [LTO] Adds a "CodeGenOnly" option. Allows the client to skip the optimizer..

This commit was part of the series to get rid of ThinLTOCodeGenerator. Removing this would makes the current state of the "new LTO" further away of the features exposed by ThinLTOCodeGenerator.

Thu, Jun 1, 5:10 PM
mehdi_amini added inline comments to D33464: [PM][WIP] Enable out-of-tree registration of passes with the new PassBuilder.
Thu, Jun 1, 5:06 PM
mehdi_amini added inline comments to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.
Thu, Jun 1, 1:48 PM
mehdi_amini added a comment to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.

Thanks for splitting out a large part of the refactoring!

Thu, Jun 1, 1:37 PM
mehdi_amini added a comment to D33125: Introduce isoneof<T0, T1, ...> as an extension of isa<T>.

Honestly, the more I think about this, the more I think that the complexity it involves doesn't pull its weight.

Thu, Jun 1, 9:34 AM

Wed, May 31

mehdi_amini added a comment to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.
Wed, May 31, 2:28 PM
mehdi_amini added a comment to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.

This flag is not serialized. We don't need it to be serialized, but that same is true for Live / LiveRoot. Actually, LiveRoot serialization was added in D23488 along with bitcode version bump, and, as far as I can see, it was unused from the start. I can kill the whole thing, and maybe even downgrade the version, if no one minds.

Wed, May 31, 2:21 PM
mehdi_amini added a comment to D21723: [RFC] Enhance synchscope representation.

To expand: my current concern is that the issue spotted with "instance" of scope makes the whole mechanism entirely opaque now, I'm not longer sure what is the point of specifying it at the IR level in a target-independent way.
@t-tye you were the one mentioning some generic understanding of the code as one of the original motivation IIRC?

Wed, May 31, 12:33 PM
mehdi_amini requested changes to D21723: [RFC] Enhance synchscope representation.

LGTM

Wed, May 31, 12:30 PM

Tue, May 30

mehdi_amini added a comment to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.

To recap: this needs to be in combined summary, because that's the only way to pass information to LowerTypeTests in regularLTO.

Tue, May 30, 4:47 PM
mehdi_amini added inline comments to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.
Tue, May 30, 2:22 PM

Mon, May 29

mehdi_amini added inline comments to rL303769: Add some tips on benchmarking..
Mon, May 29, 12:24 PM
mehdi_amini added inline comments to D33513: [ThinLTO] Fix ThinLTO crash while destroying context.
Mon, May 29, 10:25 AM · lld

Sun, May 28

mehdi_amini committed rL304130: DebugInfo: Include .dwo file name when hashing multiple CUs in a single file.
DebugInfo: Include .dwo file name when hashing multiple CUs in a single file
Sun, May 28, 11:32 PM
mehdi_amini committed rL304129: DebugInfo: Omit an empty CU when a subprogram was moved into its use.
DebugInfo: Omit an empty CU when a subprogram was moved into its use
Sun, May 28, 11:25 PM
mehdi_amini committed rL304127: IRGen: Add optnone attribute on function during O0.
IRGen: Add optnone attribute on function during O0
Sun, May 28, 10:39 PM
mehdi_amini closed D28404: IRGen: Add optnone attribute on function during O0 by committing rL304127: IRGen: Add optnone attribute on function during O0.
Sun, May 28, 10:38 PM
mehdi_amini committed rL304126: Revert "DebugInfo: Omit an empty CU when a subprogram was moved into its use".
Revert "DebugInfo: Omit an empty CU when a subprogram was moved into its use"
Sun, May 28, 10:18 PM
mehdi_amini committed rL304125: Revert "DebugInfo: Include .dwo file name when hashing multiple CUs in a single….
Revert "DebugInfo: Include .dwo file name when hashing multiple CUs in a single…
Sun, May 28, 10:18 PM

May 26 2017

mehdi_amini added a comment to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.
In D33615#766506, @pcc wrote:

Add a bit to GVFlags to tell ThinLTO backends which values are dead.

Why?

See Evgeniy's RFC thread, in particular
http://lists.llvm.org/pipermail/llvm-dev/2017-May/113298.html

May 26 2017, 10:36 PM
mehdi_amini added a comment to D33540: [PM/ThinLTO] Port the ThinLTO pipeline (both components) to the new PM..

Yeah, sorry, this all only makes sense for *partial inlining*, not for the rest of it. Anyways... Sorry for confusion.

May 26 2017, 10:32 PM
mehdi_amini added a comment to D33615: Move summary dead stripping before regular LTO and record results in the combined summary.

Add a bit to GVFlags to tell ThinLTO backends which values are dead.

May 26 2017, 10:27 PM
mehdi_amini added a comment to D33571: PMB: Run the whole-program-devirt pass during LTO at --lto-O0..

LGTM.

May 26 2017, 11:24 AM
mehdi_amini added a comment to D33540: [PM/ThinLTO] Port the ThinLTO pipeline (both components) to the new PM..

Indeed, looks like the buildModuleSimplificationPipeline got me confused with buildFunctionSimplificationPipeline, sorry /me -> []

May 26 2017, 10:05 AM
mehdi_amini added a comment to D33540: [PM/ThinLTO] Port the ThinLTO pipeline (both components) to the new PM..

Note: the plan was to tune the inliner heuristic in the compile phase differently from the thin-backends phase, but we never got to it. It is also already challenging to maintain and evolve the existing thresholds, that I'm not sure adding another "optimization goal" is sustainable.

May 26 2017, 9:50 AM
mehdi_amini added a comment to D33540: [PM/ThinLTO] Port the ThinLTO pipeline (both components) to the new PM..

For partial inlining, leave it consistent with old PM for now. We can introduce later an internal option to enable/disable pre-thin-link partial inlining and collect more performance number.

Is there a rationale for the position in the old PM? Was this discussed somewhere?

May 26 2017, 9:44 AM
mehdi_amini added inline comments to D33513: [ThinLTO] Fix ThinLTO crash while destroying context.
May 26 2017, 9:24 AM · lld

May 25 2017

mehdi_amini added a comment to D33513: [ThinLTO] Fix ThinLTO crash while destroying context.

Thanks for your first contribution, great GSoC start!

Thanks! This was a good chance to look closely at the code to start learning how it works.

One note: this manifests with ThinLTO, but it might be a more generic metadata issue. Are you able to reproduce without ThinLTO (and maybe with a single file?)

The bug only happens when lazy loading Metadata. Lazy loading occurs when module level metadata is being imported by ThinLTO (see https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp;303885$781). I believe this is the only path that leads to lazy loading, so I don't think it can happen elsewhere.

May 25 2017, 5:03 PM · lld

May 24 2017

mehdi_amini added inline comments to D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM..
May 24 2017, 5:07 PM
mehdi_amini added a reviewer for D33513: [ThinLTO] Fix ThinLTO crash while destroying context: dexonsmith.
May 24 2017, 11:52 AM · lld

May 23 2017

mehdi_amini added inline comments to D21723: [RFC] Enhance synchscope representation.
May 23 2017, 4:22 PM

May 22 2017

mehdi_amini added inline comments to D21723: [RFC] Enhance synchscope representation.
May 22 2017, 10:41 PM
mehdi_amini added inline comments to D21723: [RFC] Enhance synchscope representation.
May 22 2017, 7:51 PM

May 19 2017

mehdi_amini accepted D33370: Don't verify cross-imported bitcode in FunctionImporter.

LGTM.

May 19 2017, 4:39 PM
mehdi_amini added a comment to D33370: Don't verify cross-imported bitcode in FunctionImporter.

It turns out that the SrcModule in FunctionImporter is in a really inconsistent intermediate state at the point where I ran the Verifier.

May 19 2017, 3:30 PM
mehdi_amini accepted D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

LGTM.

May 19 2017, 10:36 AM
mehdi_amini accepted D33360: Rewrite llvm-lto's codegen() using ThinCodeGenerator::run(). NFC-ish..

Thanks!

May 19 2017, 10:32 AM
mehdi_amini accepted D32401: [Devirtualization] insert placement new barrier with -O0.

LGTM.

May 19 2017, 10:31 AM

May 17 2017

mehdi_amini accepted D33291: [ThinLTO] Do not assert when adding a module with a different but compatible target triple.

LGTM. See one inline comment.

May 17 2017, 12:14 PM
mehdi_amini added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

Thanks for the offer! Please let me know if you won't get to it. If you can just quickly sketch what needs to be done I can do the grunt work, too.

May 17 2017, 9:39 AM

May 16 2017

mehdi_amini added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Because llvm-lto invokes codegen() directly after loading the module itself (wihtout going through ThinLTOCodeGenerator for the loading):

May 16 2017, 3:44 PM
mehdi_amini added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.

In the attached version I removed the flag. This will cause the

void ThinLTOCodeGenerator::run() // CodeGenOnly=true

path to verify the module twice. I'm not particularly concerned by this though and it keeps the code simpler.

May 16 2017, 3:21 PM
mehdi_amini added inline comments to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..
May 16 2017, 2:02 PM

May 15 2017

mehdi_amini added a comment to D33162: IR: Give function GlobalValue::getRealLinkageName() a less misleading name: getPGOName()..

(thanks, with this naming it looks better to me!)

May 15 2017, 6:00 PM
mehdi_amini added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

(+@pcc)
Someone should take care of making sure this is also handled by the new LTO API.

May 15 2017, 5:19 PM
mehdi_amini updated subscribers of D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..
May 15 2017, 5:17 PM
mehdi_amini added a comment to D21723: [RFC] Enhance synchscope representation.

I think so, but what would be nice to have a test that round-trip a a few non-standard strings (i.e. not "singlethread" or "system") through MIR and through bitcode.

May 15 2017, 1:18 PM
mehdi_amini added a comment to D21723: [RFC] Enhance synchscope representation.

Is there a MIR test?

May 15 2017, 12:52 PM
mehdi_amini accepted D32803: [LTO] Print time-passes information at conclusion of LTO codegen.

LGTM, thanks.

May 15 2017, 7:59 AM

May 14 2017

mehdi_amini accepted D33177: any: Add availability for experimental::bad_any_cast.

LGTM.

May 14 2017, 7:53 PM

May 13 2017

mehdi_amini added a comment to D33162: IR: Give function GlobalValue::getRealLinkageName() a less misleading name: getPGOName()..
In D33162#754180, @pcc wrote:

Most of the uses reads quite wrong since it has nothing to do with PGO.

Yes, that's the point :)

Use of a function named getPGOName() in code that has nothing to do with PGO stands out and makes it more obvious that there is a bug. Using a "better" name would increase the chance of the function being used incorrectly in other places.

May 13 2017, 2:52 PM

May 12 2017

mehdi_amini added a comment to D33162: IR: Give function GlobalValue::getRealLinkageName() a less misleading name: getPGOName()..

Most of the uses reads quite wrong since it has nothing to do with PGO. I'm not fond of this renaming right now, can we find another alternative name?

May 12 2017, 8:43 PM
mehdi_amini added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

What about disabling the verification as part of the pass pipeline and just do the manual call to the verifier on loading?

May 12 2017, 8:39 PM
mehdi_amini added a comment to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..

The codegen-only path also does not exercise the cross-module importing.

May 12 2017, 2:31 PM
mehdi_amini added inline comments to D33151: ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator..
May 12 2017, 2:08 PM
mehdi_amini accepted D33140: LTO: Don't verify modules twice in verifyMergedModuleOnce.

LGTM

May 12 2017, 10:34 AM

May 11 2017

mehdi_amini added inline comments to D32803: [LTO] Print time-passes information at conclusion of LTO codegen.
May 11 2017, 10:39 AM

May 10 2017

mehdi_amini accepted D33030: [libcxxabi] Align unwindHeader on a double-word boundary .

If field unwindHeader is annotated with the aligned attribute, both the field and struct cxa_exception are aligned. As a result, the exception object that follows cxa_exception is aligned too.

If we annotate struct cxa_exception with the attribute instead of annotating the field, the test case segfaults. It seems that this approach doesn't work because several places in cxa_exception.cpp assume the exception object immediately follows field unwindHeader (there are should be no paddings at the end of cxa_exception).

May 10 2017, 5:13 PM