efriedma (Eli Friedman)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 10 2016, 1:07 PM (88 w, 5 d)

Recent Activity

Yesterday

Herald added a reviewer for D42759: [CGP] Split large data structres to sink more GEPs: javed.absar.

This is looking good. Just a few more small comments.

Mon, Apr 23, 6:02 PM
efriedma added a comment to D45191: [LoopReroll] Rewrite induction variable rewriting..

Ping.

Mon, Apr 23, 4:55 PM
efriedma added a comment to D45924: Set calling convention for varargs .

What branch is this patch against? Trunk doesn't have a file named MergeSimilarFunctions.cpp.

Mon, Apr 23, 4:52 PM
efriedma accepted D45971: [LoopInfo] Verify BBMap tracks innermost loops for BBs..

LGTM

Mon, Apr 23, 4:11 PM
efriedma added inline comments to D45916: Enable MachineOutliner by default under -Oz for AArch64.
Mon, Apr 23, 12:49 PM
efriedma accepted D45970: [LoopInterchange] Do not change LI for BBs in child loops..

I'm not really happy with LoopInterchangeTransform::restructureLoops overall; it seems like complicated, fragile code. But I don't see a better alternative.

Mon, Apr 23, 12:40 PM
efriedma accepted D45971: [LoopInfo] Verify BBMap tracks innermost loops for BBs..

While you're at it, could you also add a check to compareLoops to make sure the DenseBlockSet is consistent?

Mon, Apr 23, 12:35 PM
efriedma accepted D45535: [DSE] Teach the pass that atomic memory intrinsics are stores..

LGTM

Mon, Apr 23, 12:06 PM
efriedma added a comment to D45970: [LoopInterchange] Do not change LI for BBs in child loops..

Needs testcase.

Mon, Apr 23, 12:03 PM
efriedma added inline comments to D45535: [DSE] Teach the pass that atomic memory intrinsics are stores..
Mon, Apr 23, 12:00 PM
efriedma added inline comments to D45279: [LoopInterchange] Use getExitBlock()/getExitingBlock instead of manual impl..
Mon, Apr 23, 11:55 AM
efriedma accepted D45889: [MemCpyOpt] Skip optimizing basic blocks not reachable from entry.

LGTM

Mon, Apr 23, 11:43 AM

Fri, Apr 20

efriedma added a reviewer for D44928: [MC] Pass MCSubtargetInfo to fixupNeedsRelaxation and applyFixup: phosek.

I'm not sure I completely follow the bundle-related changes; adding Petr Hosek as a reviewer.

Fri, Apr 20, 5:38 PM
efriedma added a comment to D45418: [SimplifyLibcalls] Atoi, strtol replacements.

I was going to commit this, but then I discovered that str-int.ll and str-int-2.ll are identical. Did you accidentally overwrite your changes?

Fri, Apr 20, 5:17 PM
efriedma committed rL330495: [AArch64] Don't crash trying to resolve __stack_chk_guard..
[AArch64] Don't crash trying to resolve __stack_chk_guard.
Fri, Apr 20, 5:11 PM
efriedma closed D45746: [AArch64] Don't crash trying to resolve __stack_chk_guard..
Fri, Apr 20, 5:11 PM
efriedma added a comment to D45910: [BasicAA] Don't assume a PHI node has inputs.

We have a verifier check that PHI nodes have at least one incoming value, so this should be impossible even in unreachable code.

Fri, Apr 20, 5:02 PM
efriedma added a comment to D44885: [RISCV] Expand function call to "call" pseudoinstruction.

Oh, nevermind, missed the part about linker relaxation. Please add a comment to the code which mentions that.

Fri, Apr 20, 12:59 PM
efriedma added a comment to D44885: [RISCV] Expand function call to "call" pseudoinstruction.

Why do we want to do this? As far as I can tell, it leads to worse code (e.g. br_fcmp_store_load_stack_slot materializes the address of @dummy twice).

Fri, Apr 20, 12:55 PM
efriedma added a comment to D4276: Added llvm.is.constant intrinsic.

Needs a testcase for the SCCP fix. Otherwise LGTM.

Fri, Apr 20, 12:22 PM · deleted
efriedma added a comment to D45889: [MemCpyOpt] Skip optimizing basic blocks not reachable from entry.

C dominates LI because it's the result of getDependency. LI dominates SI because LI is an operand of SI. Therefore C dominates SI. LI and SI are in the same block because of the getParent() check. C is in the same block as LI because getDependency always returns an instruction in the same block. Therefore C and SI are in the same block. Given C and SI are in the same block, and C dominates SI, and the block is reachable, C must come before SI in the block (so SI can't be the first instruction in the block, and the loop will eventually terminate).

Fri, Apr 20, 12:11 PM
efriedma added inline comments to D45736: [SimplifyLibcalls] Replace locked IO with unlocked IO.
Fri, Apr 20, 12:01 PM
efriedma accepted D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.

LGTM

Fri, Apr 20, 11:25 AM
efriedma added a comment to D45835: Add new driver mode for dumping compiler options.

If any of those options we care about wind up being changed, there's a good chance we may need to change something on our end anyway, so breaking us is actually useful.

Fri, Apr 20, 11:22 AM
efriedma added a comment to D45889: [MemCpyOpt] Skip optimizing basic blocks not reachable from entry.

This sort of thing gets very confusing; it would be better to explicitly reject unreachable blocks in MemCpyOptPass::iterateOnFunction.

Fri, Apr 20, 10:49 AM

Thu, Apr 19

efriedma added a comment to D45418: [SimplifyLibcalls] Atoi, strtol replacements.

I'll merge it tomorrow.

Thu, Apr 19, 4:07 PM
efriedma accepted D45418: [SimplifyLibcalls] Atoi, strtol replacements.

LGTM

Thu, Apr 19, 12:54 PM
efriedma added a comment to D45835: Add new driver mode for dumping compiler options.

The list of features/extensions seems okay; it's information which is already available through a stable interface (specifically, clang -E). JSON also seems fine as a representation.

Thu, Apr 19, 12:43 PM
efriedma added inline comments to D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.
Thu, Apr 19, 11:39 AM
efriedma added a comment to D45821: [AArch64] improve code generation of vectors smaller than 64 bit.

I'm fine with this; AArch64 supports mostly the same operations at bitwidth 8/16/32.

Thu, Apr 19, 11:30 AM

Wed, Apr 18

efriedma added a comment to D45736: [SimplifyLibcalls] Replace locked IO with unlocked IO.

It's still possible to optimize your capt_test() example.

Wed, Apr 18, 2:30 PM
efriedma added a comment to D45418: [SimplifyLibcalls] Atoi, strtol replacements.

Can you share some of the code between optimizeAtoi and optimizeStrtol?

Wed, Apr 18, 2:27 PM
efriedma added a comment to D45736: [SimplifyLibcalls] Replace locked IO with unlocked IO.

It doesn't matter what code is in the module which is currently being processed; another module could do __attribute((constructor)) void f() { pthread_t t; pthread_create(&t, 0, puts, "foo"); } or something.

Wed, Apr 18, 2:23 PM
efriedma added a comment to D45736: [SimplifyLibcalls] Replace locked IO with unlocked IO.

I don't think there's any practical way to prove that no other thread will write to stdout.

Wed, Apr 18, 1:50 PM

Tue, Apr 17

efriedma added a comment to D44626: [InstCombine] Fold (A OR B) AND B code sequence over Phi node .

Any idea how frequently this triggers on general code (the LLVM testsuite, or something like that)?

Tue, Apr 17, 5:13 PM
efriedma added a comment to D45736: [SimplifyLibcalls] Replace locked IO with unlocked IO.

That should work, as far as I know; maybe we aren't adding the right nocapture markings to the fwrite() call?

Tue, Apr 17, 4:28 PM
efriedma updated the summary of D45746: [AArch64] Don't crash trying to resolve __stack_chk_guard..
Tue, Apr 17, 4:05 PM
efriedma created D45746: [AArch64] Don't crash trying to resolve __stack_chk_guard..
Tue, Apr 17, 4:05 PM
efriedma added a comment to D45736: [SimplifyLibcalls] Replace locked IO with unlocked IO.

So maybe somehow check if module has only static functions (+main)?

Tue, Apr 17, 3:42 PM
efriedma accepted D44833: [InstCombine] peek through bitcasted vector/array pointer GEP operand.

LGTM

Tue, Apr 17, 2:50 PM
efriedma accepted D45582: Refine the loop rotation's API.

LGTM

Tue, Apr 17, 11:44 AM
efriedma added a comment to D45601: Warn on bool* to bool conversion.

The thing about the bool*-only version is that bool pointers are rare in C++, so I'm not sure we're gaining much. But if we can't do something more general, there's still some benefit.

Tue, Apr 17, 11:17 AM
efriedma edited reviewers for D45376: Fix PR34170: Crash on inline asm with 64bit output in 32bit GPR, added: efriedma; removed: lattner.

Could you separate the "add support for matching input" part into a separate patch? I'm having trouble following what exactly you're trying to do.

Tue, Apr 17, 10:55 AM
efriedma added inline comments to D45601: Warn on bool* to bool conversion.
Tue, Apr 17, 10:37 AM

Mon, Apr 16

efriedma created D45712: [WIP] Diagnose invalid cv-qualifiers for friend decls..
Mon, Apr 16, 6:40 PM
efriedma committed rL330169: [ARM] Compute a target feature which corresponds to the ARM version..
[ARM] Compute a target feature which corresponds to the ARM version.
Mon, Apr 16, 4:56 PM
efriedma committed rC330169: [ARM] Compute a target feature which corresponds to the ARM version..
[ARM] Compute a target feature which corresponds to the ARM version.
Mon, Apr 16, 4:56 PM
efriedma closed D45240: [ARM] Compute a target feature which corresponds to the ARM version..
Mon, Apr 16, 4:56 PM
efriedma committed rCXXA330162: [libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions..
[libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions.
Mon, Apr 16, 3:19 PM
efriedma added a comment to D45191: [LoopReroll] Rewrite induction variable rewriting..

Ping

Mon, Apr 16, 3:07 PM
efriedma closed D45196: [libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions..

https://reviews.llvm.org/rL330162

Mon, Apr 16, 3:04 PM
efriedma committed rL330162: [libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions..
[libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions.
Mon, Apr 16, 3:03 PM
efriedma accepted D45383: Limit types of builtins that can be redeclared..

LGTM

Mon, Apr 16, 2:30 PM · Restricted Project
efriedma added a comment to D45649: [Polly][ScopDetect] Reject loop with multiple exit blocks..

I thought we had some support for it, somehow? Nevermind, I guess I got it confused with the ErrorBlock thing.

Mon, Apr 16, 2:24 PM · Restricted Project
efriedma added a comment to D45383: Limit types of builtins that can be redeclared..

It looks like you didn't fix the tests?

Mon, Apr 16, 2:11 PM · Restricted Project
efriedma added inline comments to D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.
Mon, Apr 16, 12:57 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Mon, Apr 16, 12:52 PM
efriedma added inline comments to D45572: [X86] Replace action Promote with Custom for operation ISD::SINT_TO_FP.
Mon, Apr 16, 12:45 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Mon, Apr 16, 12:45 PM
efriedma added a comment to D45649: [Polly][ScopDetect] Reject loop with multiple exit blocks..

Can we reduce the imapct of this by treating all exits which are outside the scop as equivalent?

Mon, Apr 16, 12:34 PM · Restricted Project
efriedma added a comment to D45383: Limit types of builtins that can be redeclared..

I don't see an A in LANGBUILTIN(__va_start, "vc**.", "nt", ALL_MS_LANGUAGES)

Mon, Apr 16, 12:19 PM · Restricted Project
efriedma added inline comments to D45279: [LoopInterchange] Use getExitBlock()/getExitingBlock instead of manual impl..
Mon, Apr 16, 12:14 PM
efriedma added a comment to D45383: Limit types of builtins that can be redeclared..

We can could add an exception to the "don't allow redeclarations with custom type-checking" rule to specifically allow redeclaring __va_start. The general rule is that we don't want user code redeclaring them because they don't have a specific signature, so our internal representation could change between releases. But __va_start has a specific fixed signature from the Microsoft SDK headers, so it should be fine to allow redeclaring it without a warning.

Mon, Apr 16, 12:09 PM · Restricted Project
efriedma added a comment to D45418: [SimplifyLibcalls] Atoi, strtol replacements.

I'd like to see a second testcase which checks our transforms on declare i32 @strtol(i8*, i8**, i32) (which is the signature on 32-bit targets).

Mon, Apr 16, 11:58 AM
efriedma accepted D45413: [SimplifyLibcalls] Realloc(null, N) -> Malloc(N).

LGTM

Mon, Apr 16, 11:14 AM

Fri, Apr 13

efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Fri, Apr 13, 4:46 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Fri, Apr 13, 4:44 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Fri, Apr 13, 2:09 PM
efriedma added inline comments to D45413: [SimplifyLibcalls] Realloc(null, N) -> Malloc(N).
Fri, Apr 13, 2:00 PM
efriedma added a comment to D4276: Added llvm.is.constant intrinsic.

I guess lowering llvm.objectsize and llvm.is.constant at the same time makes sense; okay. They should probably both be lowered earlier, though.

Fri, Apr 13, 12:54 PM · deleted
efriedma added inline comments to D45582: Refine the loop rotation's API.
Fri, Apr 13, 12:49 PM
efriedma added a comment to D4276: Added llvm.is.constant intrinsic.

It seems like you're waiting to fold llvm.is.constant until really late in the optimization pipeline; we probably want to fold it sometime in the "middle", so we get better optimization. Maybe just after function simplification passes; at that point, we're unlikely to get any more useful information about whether the argument is constant, and we want to simplify the code as much as possible before we run transforms like loop vectorization.

Fri, Apr 13, 12:16 PM · deleted
efriedma added a comment to D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR.

The fcmp opcode has no defined behavior with NaN operands in the comparisions handled in this patch.

Fri, Apr 13, 12:01 PM
efriedma accepted D45510: [RFC][AliasAnalysis][BasicAA] Return MayAlias for the pointer plus variable offset to structure object member.

Yes, this patch is correct.

Fri, Apr 13, 11:54 AM
efriedma added a comment to D45321: [atomics] Fix runtime calls for misaligned atomics.

The Linux libatomic __atomic_is_lock_free returns false for unaligned pointers, even on x86. clang must generate code which is compatible with that, so it *cannot* inline misaligned atomic operations. Given that clang can't inline misaligned atomic operations anyway, I don't see any compelling reason for compiler-rt to try to lower misaligned atomics using lock-free operations.

Fri, Apr 13, 11:46 AM
efriedma added a comment to D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.

This makes sense.

Fri, Apr 13, 11:26 AM

Thu, Apr 12

efriedma added a comment to D45510: [RFC][AliasAnalysis][BasicAA] Return MayAlias for the pointer plus variable offset to structure object member.

Ultimately the undefined behavior comes out of section 6.5.6 of the C standard: "If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined."

Thu, Apr 12, 6:42 PM
efriedma updated subscribers of D45109: Remove -cc1 option "-backend-option".

The test wasn't checking anything useful; the clang driver never passes
"-arm-long-calls" to clang -cc1.  See https://reviews.llvm.org/rL241565 .

Thu, Apr 12, 4:02 PM
efriedma committed rL329968: Fix test failure caused by r329965..
Fix test failure caused by r329965.
Thu, Apr 12, 3:56 PM
efriedma committed rC329968: Fix test failure caused by r329965..
Fix test failure caused by r329965.
Thu, Apr 12, 3:56 PM
efriedma committed rC329965: Remove -cc1 option "-backend-option"..
Remove -cc1 option "-backend-option".
Thu, Apr 12, 3:26 PM
efriedma committed rL329965: Remove -cc1 option "-backend-option"..
Remove -cc1 option "-backend-option".
Thu, Apr 12, 3:26 PM
efriedma closed D45109: Remove -cc1 option "-backend-option".
Thu, Apr 12, 3:25 PM
efriedma committed rL329961: Don't call skipModule for CFI lowering passes..
Don't call skipModule for CFI lowering passes.
Thu, Apr 12, 3:07 PM
efriedma added a comment to D45550: Use GetArgumentVector to retrieve the utf-8 encoded arguments on all platforms.

I would say it's not worth investing any effort into making the test work correctly on Python2 on Windows; as long as Python3 is doing the right thing, that's good enough. (If we really wanted to, we could avoid the charset conversion in Python2 by calling CreateProcessW using ctypes.)

Thu, Apr 12, 1:13 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Thu, Apr 12, 12:59 PM
efriedma added inline comments to D45510: [RFC][AliasAnalysis][BasicAA] Return MayAlias for the pointer plus variable offset to structure object member.
Thu, Apr 12, 12:47 PM
efriedma accepted D45551: Use GetArgumentVector to retrieve the utf-8 encoded arguments on all platforms.

LGTM

Thu, Apr 12, 12:42 PM
efriedma added inline comments to D45582: Refine the loop rotation's API.
Thu, Apr 12, 12:36 PM
efriedma added inline comments to D45413: [SimplifyLibcalls] Realloc(null, N) -> Malloc(N).
Thu, Apr 12, 12:27 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Thu, Apr 12, 12:17 PM
efriedma accepted D45514: [NEON] Support intrinsic for scalar and vector versions of the VRINTN instruction.

Okay. Then LGTM.

Thu, Apr 12, 12:10 PM
efriedma added a comment to D45550: Use GetArgumentVector to retrieve the utf-8 encoded arguments on all platforms.

When Python 3 passes the command line argument to an external command, I believe it converts arguments to the current code page

Thu, Apr 12, 12:00 PM
efriedma added a comment to D45330: [WIP][IPSCCP] Use PredicateInfo to propagate facts from cmp instructions..

I think number of instructions eliminated looks quite promising

Thu, Apr 12, 11:24 AM
efriedma added inline comments to D45204: [X86][MIPS][ARM] New machine instruction property 'isMoveReg'.
Thu, Apr 12, 11:18 AM
efriedma added a comment to D44909: [DAGCombine] (float)((int) f) --> ftrunc (PR36617).

Or, in portable C, you can do a range check: if (x <= INT_MAX && x >= INT_MIN && (double)(int)x == x).

Thu, Apr 12, 9:52 AM
efriedma added a comment to D44909: [DAGCombine] (float)((int) f) --> ftrunc (PR36617).

After this change, is there some way to express this approach in C?

Thu, Apr 12, 8:52 AM

Wed, Apr 11

efriedma added inline comments to D45510: [RFC][AliasAnalysis][BasicAA] Return MayAlias for the pointer plus variable offset to structure object member.
Wed, Apr 11, 6:37 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Wed, Apr 11, 5:20 PM
efriedma added inline comments to D45418: [SimplifyLibcalls] Atoi, strtol replacements.
Wed, Apr 11, 1:52 PM