mehdi_amini (Mehdi AMINI)
User

Projects

User does not belong to any projects.
User Since
Apr 30 2013, 5:34 PM (199 w, 4 d)

Recent Activity

Yesterday

mehdi_amini accepted D30380: Teach lit to expand glob expressions.

I believe you :)

Sat, Feb 25, 2:16 PM
mehdi_amini added inline comments to D30380: Teach lit to expand glob expressions.
Sat, Feb 25, 2:01 PM
mehdi_amini added inline comments to D30380: Teach lit to expand glob expressions.
Sat, Feb 25, 1:51 PM
mehdi_amini added a comment to D30371: [libfuzzer] use find to feed xargs.

Oh right!

Sat, Feb 25, 12:01 PM · Restricted Project
mehdi_amini accepted D30371: [libfuzzer] use find to feed xargs.

In the meantime, this seems like a strict improvement to me.

Sat, Feb 25, 11:49 AM · Restricted Project
mehdi_amini added a comment to D30371: [libfuzzer] use find to feed xargs.

Given that this problem keeps happening and is not likely to go away any time soon, I wonder if the best solution is to build some special syntax into lit that can expand globs. lit already has plenty of cases where it does text replacements, for example if you write %t in a run line, or {{.*}} in a match line.

Sat, Feb 25, 11:49 AM · Restricted Project
mehdi_amini updated subscribers of D30376: Playing with tokens OR breaking coroutines manually.
Sat, Feb 25, 10:38 AM

Fri, Feb 24

mehdi_amini added a comment to D30371: [libfuzzer] use find to feed xargs.

At least the usage of xargs seems correct now!

Fri, Feb 24, 8:53 PM · Restricted Project
mehdi_amini added a comment to D30325: [Polly] Introduce isl C++ bindings - Part 1 (smart pointer interface).

You talked about "value semantic" earlier, but we have smart pointer, and you mentioned reference counting.

I don't see a contradiction here. Value semantics can be implemented using copy-on-write.

Fri, Feb 24, 11:34 AM · Restricted Project
mehdi_amini added a comment to D30325: [Polly] Introduce isl C++ bindings - Part 1 (smart pointer interface).

You talked about "value semantic" earlier, but we have smart pointer, and you mentioned reference counting. Overall I have no idea what's going on if I were to use these class.
I assume that's fine, as long as people that are familiar with ISL are happy with it, but I'd hardly consider this a "C++ API" at this point, seeing how much it seems very intrinsically tied to "the ISL way" of handling its internal and does not seem to be expressed in a way that would match common C++ ownership management idioms.

Fri, Feb 24, 10:50 AM · Restricted Project
mehdi_amini added inline comments to D30338: [Doc] Modernize programmers manual.
Fri, Feb 24, 10:27 AM
mehdi_amini added a comment to D30325: [Polly] Introduce isl C++ bindings - Part 1 (smart pointer interface).

What big difference with std::unique_ptr (and a custom deleter) justify to rewrite so much code for your smart pointer?

Fri, Feb 24, 10:16 AM · Restricted Project
mehdi_amini added a comment to D30325: [Polly] Introduce isl C++ bindings - Part 1 (smart pointer interface).

S.copy() returns a raw pointer refering to a copy of S.

Fri, Feb 24, 9:24 AM · Restricted Project
mehdi_amini accepted D30338: [Doc] Modernize programmers manual.

LGTM.

Fri, Feb 24, 9:15 AM

Thu, Feb 23

mehdi_amini added a comment to D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz.

(Though i quibble with this part:

// The called function could have undefined behavior or
// side-effects, even if marked readnone nounwind.

I'm not sure what we think those side effects are, but we should be modeling them or ignoring them.

Thu, Feb 23, 4:41 PM
mehdi_amini added a comment to D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz.

I'd add also that isSafeToSpeculativelyExecute may be conservative right now, but it should improve with https://reviews.llvm.org/D20116
So it is even unclear to me which cases GVN hoist would be able to catch that are not in the realm of isSafeToSpeculativelyExecute.

Currently isSafeToSpeculativelyExecute returns false for Calls, stores. GVN Hoist will hoist them if it is legal to do so.

Thu, Feb 23, 4:34 PM
mehdi_amini added a comment to D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz.
In D29092#682961, @hans wrote:

David, do you have any further concerns? Otherwise I'd like to see this committed so we can resolve the PR for 4.0.

I don't understand why we cannot assert isSafeToSpeculativelyExecute if we are not in minsize. If we are given a call instruction, why do we think it is safe to hoist it?

A call without any side effects, for example, can be hoisted.

Thu, Feb 23, 4:12 PM
mehdi_amini added a comment to D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz.

I'd add also that isSafeToSpeculativelyExecute may be conservative right now, but it should improve with https://reviews.llvm.org/D20116
So it is even unclear to me which cases GVN hoist would be able to catch that are not in the realm of isSafeToSpeculativelyExecute.

Thu, Feb 23, 1:24 PM
mehdi_amini added a comment to D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz.

So it seems that GVN-hoist tries to be "smarter" and hoist things for which isSafeToSpeculativelyExecute is returning false.
This is a nice aggressive optimization, but if it isn't safe (it seems it is not right now considering there are bugs in 4.0), we should *disable* this behavior in clang-4.0 and GVN-hoist should rely *only* on isSafeToSpeculativelyExecute as a legality criteria.

Thu, Feb 23, 1:21 PM
mehdi_amini added inline comments to D30277: [COFF] added test for thinlto.
Thu, Feb 23, 1:15 PM
mehdi_amini added a comment to D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz.
In D29092#682961, @hans wrote:

David, do you have any further concerns? Otherwise I'd like to see this committed so we can resolve the PR for 4.0.

I don't understand why we cannot assert isSafeToSpeculativelyExecute if we are not in minsize. If we are given a call instruction, why do we think it is safe to hoist it?

Thu, Feb 23, 1:13 PM
mehdi_amini added a comment to D30307: Fix insertion of `sanitizer_cov_trace_pc_guard` insertion in optimized code with debug info.
In D30307#684976, @kcc wrote:

LGTM
(but not claiming to understand the problem)

Thu, Feb 23, 1:03 PM
mehdi_amini updated the diff for D30307: Fix insertion of `sanitizer_cov_trace_pc_guard` insertion in optimized code with debug info.

fix test to fail the verifier

Thu, Feb 23, 12:58 PM
mehdi_amini added a comment to D30307: Fix insertion of `sanitizer_cov_trace_pc_guard` insertion in optimized code with debug info.
In D30307#684972, @kcc wrote:

is it possible to add an assert() that fails now and gets fixed with your patch?

Thu, Feb 23, 12:43 PM
mehdi_amini created D30307: Fix insertion of `sanitizer_cov_trace_pc_guard` insertion in optimized code with debug info.
Thu, Feb 23, 12:04 PM
mehdi_amini added a comment to D30277: [COFF] added test for thinlto.

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.

That was my original plan, but it looks like llvm-nm only tells us about main. Is that enough? Is there a way to get it to give us more information? I wrote the test the way I wrote it because I felt just looking at the output from llvm-nm doesn't really tell us if ThinLTO is working as intended.

Thu, Feb 23, 11:53 AM

Wed, Feb 22

mehdi_amini added inline comments to D30277: [COFF] added test for thinlto.
Wed, Feb 22, 4:59 PM
mehdi_amini added a comment to D30277: [COFF] added test for thinlto.

Maybe the testing is obvious to someone use to COFF, in which case I'm just out-of-place :)
Otherwise I'd rather have comment explaining what every check is doing.

Wed, Feb 22, 3:15 PM
mehdi_amini accepted D29999: [InlineFunction] add nonnull assumptions based on argument attributes.

LGTM. Thanks.

Wed, Feb 22, 12:03 PM
mehdi_amini added inline comments to D30240: enable building with LTO on Windows using clang-cl and lld.
Wed, Feb 22, 11:48 AM

Tue, Feb 21

mehdi_amini added a comment to D30239: enable -flto=thin in clang-cl.
In D30239#683116, @pcc wrote:

Can you please add a ThinLTO test to lld before adding driver support?

Tue, Feb 21, 8:40 PM

Mon, Feb 20

mehdi_amini added inline comments to D30086: Add generic IR vector reductions.
Mon, Feb 20, 10:25 AM
mehdi_amini added inline comments to D30167: [Assembler] Add test for !srcloc references in assembler diags.
Mon, Feb 20, 10:16 AM

Sun, Feb 19

mehdi_amini added inline comments to D30086: Add generic IR vector reductions.
Sun, Feb 19, 1:39 PM
mehdi_amini added inline comments to D30086: Add generic IR vector reductions.
Sun, Feb 19, 10:27 AM
mehdi_amini added a comment to D30086: Add generic IR vector reductions.

According to the code that I see, all reduction intrinsics have the same form and the same handling.
IMO, llvm.vector.reduce(<metadata>, <vector>) would cover all aspects of FP modes, and integer signed-unsigned variants.
The <metadata> class can include opcode and "properties".

Sun, Feb 19, 10:25 AM

Sat, Feb 18

mehdi_amini added a comment to D29998: Add initial support for debug counting.

I think some documentation in the dev guide would be welcome!

Sat, Feb 18, 4:01 PM

Fri, Feb 17

mehdi_amini added a comment to D29104: Add !associated metadata..

The IR part (LangRef) seems OK to me.
The ELF part is out of my scope, if rafael or pcc already checked it, that's fine.

Fri, Feb 17, 3:11 PM
mehdi_amini added inline comments to D29104: Add !associated metadata..
Fri, Feb 17, 3:07 PM
mehdi_amini added inline comments to D29104: Add !associated metadata..
Fri, Feb 17, 3:00 PM
mehdi_amini added inline comments to D29104: Add !associated metadata..
Fri, Feb 17, 2:42 PM
mehdi_amini added inline comments to D30086: Add generic IR vector reductions.
Fri, Feb 17, 9:52 AM
mehdi_amini added a comment to D29104: Add !associated metadata..

Can this metadata be freely dropped? Clarify this in LangRef please.

Fri, Feb 17, 9:45 AM

Thu, Feb 16

mehdi_amini added a comment to D30053: Add function importing info from samplepgo profile to the module summary..

Is it better to introduce a new meta data for this, e.g. MD_inline_instance_imports ? Can this information be directly communicated to function importer instead of relying on modifying Callgraph/profile data?

Thu, Feb 16, 5:30 PM
mehdi_amini added inline comments to D30053: Add function importing info from samplepgo profile to the module summary..
Thu, Feb 16, 5:04 PM
mehdi_amini added inline comments to D30053: Add function importing info from samplepgo profile to the module summary..
Thu, Feb 16, 4:51 PM
mehdi_amini added inline comments to D30053: Add function importing info from samplepgo profile to the module summary..
Thu, Feb 16, 4:01 PM
mehdi_amini added inline comments to D30053: Add function importing info from samplepgo profile to the module summary..
Thu, Feb 16, 3:01 PM
mehdi_amini added a comment to D30053: Add function importing info from samplepgo profile to the module summary..

You need to update: http://llvm.org/docs/BranchWeightMetadata.html

Thu, Feb 16, 2:38 PM

Wed, Feb 15

mehdi_amini accepted D30014: opt: Rename -default-data-layout flag to -data-layout and make it always override the layout..

OK

Wed, Feb 15, 7:28 PM
mehdi_amini added a comment to D30014: opt: Rename -default-data-layout flag to -data-layout and make it always override the layout..

Why?

Wed, Feb 15, 7:21 PM
mehdi_amini accepted D30008: PMB: Add an importing WPD pass to the start of the ThinLTO backend pipeline..

Make sense. LGTM.

Wed, Feb 15, 3:48 PM
mehdi_amini added a comment to D29998: Add initial support for debug counting.

A few notes:

  1. I have scripts that automate binary searching the counters that i'll upload at some point.
Wed, Feb 15, 11:21 AM
mehdi_amini accepted D29475: [LTO] Add ability to emit assembly to new LTO API.

LGTM

Wed, Feb 15, 11:16 AM

Tue, Feb 14

mehdi_amini added a comment to D13330: Implement __attribute__((unique_instantiation)).

I think this attribute is poorly named. Explicit instantiation definitions are *already* required to be globally unique; see [temp.spec]/5.1:

"For a given template and a given set of template-arguments, an explicit instantiation definition shall appear at most once in a program"

Tue, Feb 14, 4:21 PM

Mon, Feb 13

mehdi_amini committed rL295024: [ThinLTO] Make a copy of buffer identifier in ThinLTOCodeGenerator.
[ThinLTO] Make a copy of buffer identifier in ThinLTOCodeGenerator
Mon, Feb 13, 9:01 PM
mehdi_amini accepted D29701: ThinLTOBitcodeWriter: Write available_externally copies of VCP eligible functions to merged module..
In D29701#675840, @pcc wrote:
In D29701#675809, @pcc wrote:
In D29701#670366, @pcc wrote:

Note: I patched in https://github.com/pcc/llvm-project/commit/5a5904d6721f895eafdd2fc476872b98806c36e6 to measure perf impact. Total wall time spent in addRegularLTO when linking chrome was 8.2743s, as compared to about 6 seconds before (see D27324).

So >30% overhead on addRegularLTO, but what is the overhead for runRegularLTO?

Just did a fresh set of runs on chrome with https://github.com/pcc/llvm-project/commits/lto-timers patched in, and took median of 5 for each measurement:

               before after
addRegularLTO 8.0407s 9.7432s (+21%)
runRegularLTO 9.6064s 10.4748s (+9%)
Mon, Feb 13, 7:30 PM
mehdi_amini committed rL295018: [ThinLTO] Make a copy of buffer identifier in ThinLTOCodeGenerator.
[ThinLTO] Make a copy of buffer identifier in ThinLTOCodeGenerator
Mon, Feb 13, 6:32 PM
mehdi_amini added inline comments to D29781: Add alias canonicalization pass when preparing for ThinLTO.
Mon, Feb 13, 6:22 PM
mehdi_amini added a comment to D29919: allow migrating away from cmake option for LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING.

Adding @bruno to check on the module story here, I'm still not sure what is or isn't reasonable to do with modules!

Mon, Feb 13, 6:00 PM
mehdi_amini added a reviewer for D29919: allow migrating away from cmake option for LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING: bruno.
Mon, Feb 13, 5:59 PM
mehdi_amini accepted D29918: Add StringRef::getAsDouble.

LGTM.

Mon, Feb 13, 5:29 PM
mehdi_amini added inline comments to D29918: Add StringRef::getAsDouble.
Mon, Feb 13, 5:15 PM
mehdi_amini added a comment to D28404: IRGen: Add optnone attribute on function during O0.

Also note that @chandlerc in r290398 made clang adding "noinline" on every function at O0 by default, which seems very similar to what I'm doing here.

Mon, Feb 13, 3:00 PM
mehdi_amini added inline comments to D29808: WholeProgramDevirt: Add any unsuccessful llvm.type.checked.load devirtualizations to the list of llvm.type.test users..
Mon, Feb 13, 2:45 PM
mehdi_amini added inline comments to D29808: WholeProgramDevirt: Add any unsuccessful llvm.type.checked.load devirtualizations to the list of llvm.type.test users..
Mon, Feb 13, 2:10 PM
mehdi_amini added inline comments to D29808: WholeProgramDevirt: Add any unsuccessful llvm.type.checked.load devirtualizations to the list of llvm.type.test users..
Mon, Feb 13, 1:44 PM

Sun, Feb 12

mehdi_amini added a comment to D26348: Allow convergent attribute for function arguments.

I've given it a little more thought during coffee break.

divergent_branch, divergent_index, convergent_branch & convergent_index aren't enough.
At least one more modifier is needed: cannot_diverge (or similar name).

Sun, Feb 12, 2:58 PM
mehdi_amini added a comment to D26348: Allow convergent attribute for function arguments.

Currently, the problem is that right now LLVM in the latter case is "optimizing" the code to execute like the former code to save one image_sample instruction, which is wrong.

Sun, Feb 12, 2:41 PM
mehdi_amini added inline comments to D29808: WholeProgramDevirt: Add any unsuccessful llvm.type.checked.load devirtualizations to the list of llvm.type.test users..
Sun, Feb 12, 2:01 PM

Sat, Feb 11

mehdi_amini committed rL294870: Update Kaleidoscope tutorial and improve Windows support.
Update Kaleidoscope tutorial and improve Windows support
Sat, Feb 11, 1:38 PM
mehdi_amini closed D29864: Update Kaleidoscope tutorial and improve Windows support by committing rL294870: Update Kaleidoscope tutorial and improve Windows support.
Sat, Feb 11, 1:38 PM
mehdi_amini added a comment to D29864: Update Kaleidoscope tutorial and improve Windows support.

Do you have commit access?

Sat, Feb 11, 1:26 PM
mehdi_amini accepted D29864: Update Kaleidoscope tutorial and improve Windows support.

LGTM overall. I trust you to have tested the windows part.

Sat, Feb 11, 1:26 PM
mehdi_amini added inline comments to D29864: Update Kaleidoscope tutorial and improve Windows support.
Sat, Feb 11, 11:04 AM

Fri, Feb 10

mehdi_amini added inline comments to D29701: ThinLTOBitcodeWriter: Write available_externally copies of VCP eligible functions to merged module..
Fri, Feb 10, 9:30 PM
mehdi_amini added a comment to D29701: ThinLTOBitcodeWriter: Write available_externally copies of VCP eligible functions to merged module..
In D29701#670366, @pcc wrote:

Note: I patched in https://github.com/pcc/llvm-project/commit/5a5904d6721f895eafdd2fc476872b98806c36e6 to measure perf impact. Total wall time spent in addRegularLTO when linking chrome was 8.2743s, as compared to about 6 seconds before (see D27324).

Fri, Feb 10, 9:23 PM
mehdi_amini added inline comments to D29734: IR: Function summary extensions for whole-program devirtualization pass..
Fri, Feb 10, 6:10 PM
mehdi_amini added a comment to D29782: IR: Type ID summary extensions for WPD; thread summary into WPD pass..

LGTM

Fri, Feb 10, 6:06 PM
mehdi_amini added inline comments to D29734: IR: Function summary extensions for whole-program devirtualization pass..
Fri, Feb 10, 5:36 PM
mehdi_amini added a comment to D27855: try to extend nonnull-ness of arguments from a callsite back to its parent function.

So as to unblock this review, I suggest that we introduce this change behind an off by default flag. Separately, we should pursue the clang side changes need to enable this flag by default. As stated previously, this will benefit my frontend and I'd like to see it land so that I can enable it.

Fri, Feb 10, 10:03 AM
mehdi_amini committed rL294759: Fix doc for `-opt-bisect-limit`: the LTO option prefix for lld is -mllvm.
Fix doc for `-opt-bisect-limit`: the LTO option prefix for lld is -mllvm
Fri, Feb 10, 9:27 AM
mehdi_amini added a comment to D29376: LTO: align the Monolithic LTO optimization pipeline on the ThinLTO and O2/O3 one.

Thanks for the update. Good to know.

Fri, Feb 10, 9:23 AM

Thu, Feb 9

mehdi_amini added a comment to D28404: IRGen: Add optnone attribute on function during O0.

Ping :)

Thu, Feb 9, 11:35 PM
mehdi_amini committed rL294725: Fix doc for `-opt-bisect-limit`: the LTO option is linker specific.
Fix doc for `-opt-bisect-limit`: the LTO option is linker specific
Thu, Feb 9, 11:32 PM
mehdi_amini added a comment to D29799: First cut at moving driver tools into their own files..
In D29799#673004, @dlj wrote:

OK, thanks for the cleanup! I hate these infinitely long files :)
(I wouldn't bother with the svn attributes, but if it is trivial *shrug*)

I'll take this as volunteering to review. :-)

Thu, Feb 9, 7:07 PM
mehdi_amini committed rL294696: Fully qualify (preprend ::) calls to math functions from libc.
Fully qualify (preprend ::) calls to math functions from libc
Thu, Feb 9, 6:56 PM
mehdi_amini closed D29804: Fully qualify (preprend ::) calls to math functions from libc by committing rL294696: Fully qualify (preprend ::) calls to math functions from libc.
Thu, Feb 9, 6:56 PM
mehdi_amini created D29804: Fully qualify (preprend ::) calls to math functions from libc.
Thu, Feb 9, 6:15 PM
mehdi_amini added a comment to D27855: try to extend nonnull-ness of arguments from a callsite back to its parent function.

That's what I'd like to know. There was no reply to my question here or the pings in D27114. I'm hoping to hear that the clang blocker is in someone's queue?

Thu, Feb 9, 5:36 PM
mehdi_amini added a comment to D29799: First cut at moving driver tools into their own files..

OK, thanks for the cleanup! I hate these infinitely long files :)
(I wouldn't bother with the svn attributes, but if it is trivial *shrug*)

Thu, Feb 9, 5:22 PM
mehdi_amini added a comment to D29799: First cut at moving driver tools into their own files..

cfg-commits not subscribed apparently? Is this supposed to be up for review?

Thu, Feb 9, 4:47 PM
mehdi_amini added a comment to D29472: [ARM] Add support for armv7ve triple in llvm (PR31358)..

Yeah ignore me please :)

Thu, Feb 9, 3:05 PM

Wed, Feb 8

mehdi_amini added a comment to D27855: try to extend nonnull-ness of arguments from a callsite back to its parent function.

Aren't we blocked on clang? Or did it already stop emitting the null attr for memcpy and cie.

Wed, Feb 8, 5:40 PM
mehdi_amini added a comment to D29376: LTO: align the Monolithic LTO optimization pipeline on the ThinLTO and O2/O3 one.

Awesome! Thanks for running it, this is not surprising at all :)

Wed, Feb 8, 6:01 AM
mehdi_amini added a comment to D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size.

Ping? This is hitting 4.0

Wed, Feb 8, 1:15 AM
mehdi_amini accepted D29693: Fix bitcode upgrade for DIGlobalVariables with a var: field..

Great: at least we increased the coverage :)

Wed, Feb 8, 12:58 AM
mehdi_amini added a comment to D28913: ThinLTOBitcodeWriter: Strip debug info from merged module..
In D28913#670212, @pcc wrote:

(FWIW, I wouldn't necessarily call this a "temporary" approach. I'd say that the benefit of adding the flag to CloneModule should first be proven through benchmarking.)

Wed, Feb 8, 12:52 AM
mehdi_amini accepted D28913: ThinLTOBitcodeWriter: Strip debug info from merged module..

I thought after discussing this on IRC this had landed two weeks ago! Apparently I didn't click approve here...

Wed, Feb 8, 12:48 AM

Mon, Feb 6

mehdi_amini accepted D29349: Fix the bitcode upgrade for DIGlobalVariable in a DIImportedEntity context..

LGTM. Thanks!

Mon, Feb 6, 9:50 PM