efriedma (Eli Friedman)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

efriedma added a comment to D33442: [ARM] Fix lowering of misaligned memcpy/memset.

Please regenerate the tests in a separate commit, to make it clear what this patch is actually changing. Please change the description to make it clear what this is actually fixing; as far as I can tell, we aren't generating "wrong code", just inlining memcpy and memset calls too aggressively.

Tue, May 23, 11:31 AM
efriedma added a comment to D33446: [ARM] Temporarily disable globals promotion to constant pools to prevent miscompilation.

Do you confirm that such behavior is acceptable?

Tue, May 23, 11:11 AM
efriedma added inline comments to D33391: [DAGCombine] fold (add/uaddo (xor a, -1), 1) -> (sub 0, a).
Tue, May 23, 11:06 AM
efriedma added a comment to D33446: [ARM] Temporarily disable globals promotion to constant pools to prevent miscompilation.

I would suggest switching the option (arm-promote-constant) to default to false rather than commenting out the code, so we don't have to XFAIL the unit-tests.

Tue, May 23, 10:36 AM

Yesterday

efriedma added inline comments to D30107: Refactor DynamicLibrary so searching for a symbol will have a defined order and libraries are properly unloaded when llvm_shutdown is called..
Mon, May 22, 4:58 PM
efriedma added inline comments to D30107: Refactor DynamicLibrary so searching for a symbol will have a defined order and libraries are properly unloaded when llvm_shutdown is called..
Mon, May 22, 4:57 PM
efriedma added inline comments to D33391: [DAGCombine] fold (add/uaddo (xor a, -1), 1) -> (sub 0, a).
Mon, May 22, 2:53 PM

Fri, May 19

efriedma added a reviewer for D32998: [SROA] enable splitting for non-whole-alloca loads and stores: chandlerc.
Fri, May 19, 12:37 PM
efriedma added inline comments to D30107: Refactor DynamicLibrary so searching for a symbol will have a defined order and libraries are properly unloaded when llvm_shutdown is called..
Fri, May 19, 12:26 PM
efriedma accepted D32839: SimplifyLibCalls: Optimize wcslen.

LGTM.

Fri, May 19, 12:10 PM

Thu, May 18

efriedma added a comment to D32993: DAGCombine: Extend createBuildVecShuffle for case len(in_vec) = 4*len(result_vec).

Conservatively bail out if (min_mask_index*2 > NumElem || max_mask_index * 2 < NumElems) which means that we are accessing elements from one half of the input vector

Thu, May 18, 2:52 PM
efriedma added inline comments to D24805: [GVNSink] Initial GVNSink prototype.
Thu, May 18, 2:09 PM
efriedma added inline comments to D30107: Refactor DynamicLibrary so searching for a symbol will have a defined order and libraries are properly unloaded when llvm_shutdown is called..
Thu, May 18, 1:27 PM
efriedma added inline comments to D32993: DAGCombine: Extend createBuildVecShuffle for case len(in_vec) = 4*len(result_vec).
Thu, May 18, 11:40 AM
efriedma edited reviewers for D24805: [GVNSink] Initial GVNSink prototype, added: efriedma; removed: eli.friedman.
Thu, May 18, 11:22 AM

Wed, May 17

efriedma accepted D28637: [PPC] Inline expansion of memcmp.

Looks good! A few minor comments. Please test carefully before merging.

Wed, May 17, 12:07 PM
efriedma added a comment to D33240: [Atomics] Rename and change prototype for atomic memcpy intrinsic.

I don't know enough about the possible source languages to know with 100% certainty that it's not possible to mix, say, unordered loads with ordered stores.

Wed, May 17, 11:02 AM

Tue, May 16

efriedma accepted D33247: [InstCombine] add isCanonicalPredicate() helper function and use it; NFCI.

Cleanup looks fine.

Tue, May 16, 4:04 PM
efriedma accepted D33221: [InstSimplify] add folds for constant mask of value shifted by constant.

Looks good.

Tue, May 16, 12:12 PM
efriedma resigned from D33237: [GSoC] Shell autocompletion for clang.
Tue, May 16, 12:09 PM
efriedma added a comment to D33240: [Atomics] Rename and change prototype for atomic memcpy intrinsic.

Do we get any practical benefit from separately specifying whether the source and destination require unordered operations?

Tue, May 16, 11:31 AM

Mon, May 15

efriedma added a comment to D33221: [InstSimplify] add folds for constant mask of value shifted by constant.

We don't want to write out a bunch of patterns here which are completely redundant, but this pattern in particular is likely to come up in code involving bitfields, so it seems fine.

Mon, May 15, 6:59 PM
efriedma added a comment to D33074: InstCombine: Allow sinking instructions with more uses in the same block..

Typically, canonicalization wants things *hoisted* not *sunk*, to make the values available for redundancy elimination.

Mon, May 15, 6:29 PM

Wed, May 10

efriedma added a comment to D33076: [PPC] Move the combine "a << (b % (sizeof(a) * 8)) -> (PPCshl a, b)" to the backend. NFC..

Looks fine, but someone who knows PPC better should approve.

Wed, May 10, 4:25 PM
efriedma added a comment to D33076: [PPC] Move the combine "a << (b % (sizeof(a) * 8)) -> (PPCshl a, b)" to the backend. NFC..

Please update the comment in PPCISelLowering.h

Wed, May 10, 3:31 PM
efriedma added a comment to D32742: [CodeGen] JumpMaps switch statement optimization (stub).

(I'm planning to look at this and add some comments; expect a reply next week.)

Wed, May 10, 1:44 PM
efriedma added a comment to D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

Err, oh, meant to add one minor comment: please rename "method-bad-param.mm" to "interface-return-type.mm".

Wed, May 10, 1:13 PM
efriedma accepted D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

LGTM

Wed, May 10, 1:13 PM
efriedma added a comment to D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

Err, I meant the one I wrote earlier in this thread: https://reviews.llvm.org/D32759#748007

Wed, May 10, 12:26 PM
efriedma added a comment to D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

Our behavior for your testcase is less than ideal (we should be generating an invalid declaration instead of just dropping it on the floor), but that's not something you need to fix here.

Wed, May 10, 12:18 PM
efriedma accepted D30471: [SDAG] Relax conditions under stores of loaded values can be merged.

LGTM.

Wed, May 10, 11:08 AM
efriedma accepted D33050: [InstCombine] remove fold that swaps xor/or with constants.

It's a canonicalization of sorts; could help pick up more complicated patterns like ((a ^ c1) | c2) ^ c3. Please make sure we have a regression test like this for instcombine.

Wed, May 10, 11:05 AM
efriedma added a comment to D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

The testcase I wrote should change behavior, I think.

Wed, May 10, 8:32 AM
efriedma added a comment to D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

A testcase would be nice... but otherwise, yes.

Wed, May 10, 8:29 AM

Tue, May 9

efriedma added inline comments to D30107: Refactor DynamicLibrary so searching for a symbol will have a defined order and libraries are properly unloaded when llvm_shutdown is called..
Tue, May 9, 2:58 PM
efriedma added inline comments to D32990: [NewGVN] Take in account incoming edges computing congruent PhiExpression(s).
Tue, May 9, 11:38 AM
efriedma added inline comments to D32990: [NewGVN] Take in account incoming edges computing congruent PhiExpression(s).
Tue, May 9, 11:19 AM
efriedma added inline comments to D30471: [SDAG] Relax conditions under stores of loaded values can be merged.
Tue, May 9, 11:11 AM
efriedma edited reviewers for D32993: DAGCombine: Extend createBuildVecShuffle for case len(in_vec) = 4*len(result_vec), added: efriedma; removed: eli.friedman.
Tue, May 9, 8:32 AM

Mon, May 8

efriedma added a comment to D32982: CodeGenModule: Always output wchar_size; check LLVM assumptions..

This makes sense to me, but I'll wait to see if anyone else has a comment.

Mon, May 8, 3:54 PM
efriedma added a comment to D32839: SimplifyLibCalls: Optimize wcslen.

clang only emits wchar_size on ARM at the moment; that might need to change before this gets merged.

Mon, May 8, 2:33 PM
efriedma added a comment to D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

So when we see T->isObjCObjectType() is true, then we should return true since the return type is invalid?

Mon, May 8, 12:18 PM
efriedma added a comment to D32564: AArch64: compress jump tables to minimum size needed to reach destinations.

On 5/8/2017 9:36 AM, Tim Northover wrote:

On 26 April 2017 at 16:33, Eli Friedman via Phabricator
<reviews@reviews.llvm.org> wrote:

If you're looking to save size, we could put the jump table into the text segment just after the branch, and compute the jump destination in three instructions: one to load the address of the table, one to load the offset from the table, and one to add the offset to the address of the table. Is there some reason we shouldn't do that?

To get that first load down to 1 instruction we'd need to guarantee
the jump table was within 128MB of the code, and I think we
technically support larger functions. So we'd have to add something
like ARM's full Constant Island pass instead of the newly generic
branch relaxation we currently do. I don't think that is worth the
maintenance burden, it's a constant source of bugs on ARM.

For the second offset addition, again you're compromising distance.
The jump table itself then has to be within 256 or 65536 bytes of
every basic block used. I suspect that even *with* complex placement
that would be tough to arrange.

Mon, May 8, 12:17 PM

Sat, May 6

efriedma added a comment to D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint.

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

Sat, May 6, 9:41 AM

Fri, May 5

efriedma added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Fri, May 5, 1:41 PM

Thu, May 4

efriedma added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Thu, May 4, 2:16 PM
efriedma added a reviewer for D32839: SimplifyLibCalls: Optimize wcslen: dblaikie.
Thu, May 4, 1:54 PM
efriedma accepted D32874: [JumpThreading] Change a dyn_cast that is already protected by an isa check to a static cast. Combine the with another static cast. NFC.

Oh, oops, you're right. Could you change it to check isVectorTy() instead, to make that more obvious?

Thu, May 4, 1:25 PM
efriedma added inline comments to D32874: [JumpThreading] Change a dyn_cast that is already protected by an isa check to a static cast. Combine the with another static cast. NFC.
Thu, May 4, 12:56 PM
efriedma accepted D32876: [InstSimplify] add folds for or-of-casted-icmps.

The second testcase isn't getting folded because ConstantFoldCastOperand isn't getting called (you call ConstantExpr::getCast and never attempt to fold it). IIRC there was a thread recently about a similar problem with InstSimplify of GEPs.

Thu, May 4, 12:27 PM
efriedma added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Thu, May 4, 12:15 PM
efriedma added inline comments to D32839: SimplifyLibCalls: Optimize wcslen.
Thu, May 4, 11:34 AM

Wed, May 3

efriedma accepted D32789: [ScopInfo] Do not use LLVM names to identify statements.

LGTM.

Wed, May 3, 1:16 PM · Restricted Project
efriedma accepted D32596: [DAGCombine] Transform (fadd A, (fmul B, -2.0)) -> (fsub A, (fadd B, B))..

LGTM.

Wed, May 3, 1:12 PM
efriedma added inline comments to D32596: [DAGCombine] Transform (fadd A, (fmul B, -2.0)) -> (fsub A, (fadd B, B))..
Wed, May 3, 12:20 PM
efriedma accepted D32281: [ARM] ACLE Chapter 9 intrinsics.

LGTM.

Wed, May 3, 11:31 AM
efriedma added a comment to D32789: [ScopInfo] Do not use LLVM names to identify statements.

This looks very similar to a change we made internally (down to the name; we call it -polly-use-llvm-names). We originally did it because we were having trouble with differences in generated code between Release and Release+Asserts builds, but the performance improvement is a nice side-effect.

Wed, May 3, 11:22 AM · Restricted Project

Tue, May 2

efriedma added reviewers for D32596: [DAGCombine] Transform (fadd A, (fmul B, -2.0)) -> (fsub A, (fadd B, B)).: RKSimon, craig.topper, arsenm.

Not sure if we need a target hook for this...? Replacing one instruction with two might not always be a good idea.

Tue, May 2, 12:57 PM
efriedma added a comment to D32725: [InstCombine] Apply deMorgan to (and/or (not cmp1), cmp2) when cmp1 has multiple uses, but cmp2 has a single use.

I'm not quite sure about this the way it's written. This doesn't reduce the total number of instructions on its own, and it seems like an overly complicated pattern. You could approach this from a different angle: fold xor(icmp(pred, x, y)) to (icmp (!pred, x, y)) even if the inner icmp has multiple uses. Or maybe this is okay as-is; my intuition about what's right here isn't strong.

Tue, May 2, 11:46 AM
efriedma added inline comments to D32729: LV: Don't vectorize with unknown loop counts on divergent targets.
Tue, May 2, 11:16 AM
efriedma accepted D32674: [LoopIdiom] PR32811 check for safety while expanding.

LGTM, with some minor comments.

Tue, May 2, 10:13 AM

Mon, May 1

efriedma accepted D32665: [InstCombine] don't use DeMorgan's Law on integer constants.

LGTM.

Mon, May 1, 4:06 PM
efriedma added inline comments to D32674: [LoopIdiom] PR32811 check for safety while expanding.
Mon, May 1, 2:11 PM
efriedma added inline comments to D32674: [LoopIdiom] PR32811 check for safety while expanding.
Mon, May 1, 12:08 PM
efriedma accepted D32703: [InstCombine] check one-use before applying DeMorgan nor/nand folds.

LGTM.

Mon, May 1, 11:56 AM

Fri, Apr 28

efriedma added a comment to D32665: [InstCombine] don't use DeMorgan's Law on integer constants.

Your reasoning makes sense.

Fri, Apr 28, 10:10 PM

Thu, Apr 27

efriedma added a comment to D32596: [DAGCombine] Transform (fadd A, (fmul B, -2.0)) -> (fsub A, (fadd B, B))..

Another data-point: reassociate currently prefers to canonicalize "fadd %x, %x", to "fmul %x, 2". We don't want to fight back and forth in IR.

Thu, Apr 27, 5:59 PM
efriedma resigned from D32391: [SelectionDAG] Improve support for promotion of <1 x fX> floating point argument types (PR31088).

(I don't understand how this is supposed to work, so I don't feel comfortable approving it.)

Thu, Apr 27, 3:01 PM
efriedma added inline comments to D32563: Add LiveRangeShrink pass to shrink live range within BB..
Thu, Apr 27, 1:26 PM
efriedma committed rL301575: [GlobalOpt] Correctly update metadata when localizing a global..
[GlobalOpt] Correctly update metadata when localizing a global.
Thu, Apr 27, 11:52 AM
efriedma closed D32551: [GlobalOpt] Correctly update metadata when localizing a global. by committing rL301575: [GlobalOpt] Correctly update metadata when localizing a global..
Thu, Apr 27, 11:52 AM
efriedma added a comment to D32281: [ARM] ACLE Chapter 9 intrinsics.

It would be nice to have an ARMV5TE test to make sure that the intrinsics that are supposed to work for that target actually do.

Thu, Apr 27, 11:45 AM
efriedma added a comment to D32596: [DAGCombine] Transform (fadd A, (fmul B, -2.0)) -> (fsub A, (fadd B, B))..

I would guess we should prefer fmul as the canonical form, for the same reason we prefer "shl %x, 1" over "add %x, %x": we optimize instructions where hasOneUse() is true more aggressively. I guess it doesn't make a big difference either way, though.

Thu, Apr 27, 11:34 AM

Wed, Apr 26

efriedma added inline comments to D32567: [ARM] Miscompilation on arrays promoted to constant pools.
Wed, Apr 26, 5:37 PM
efriedma added a comment to D32564: AArch64: compress jump tables to minimum size needed to reach destinations.

If I'm following correctly, for PIC small-code-model code (which is what mostly matters these days), the comparison is this:

Wed, Apr 26, 4:33 PM
efriedma added a comment to D32552: [LibCallsShrinkWrap] Teach the pass how to preserve the dominator.

And use getAnalysisIfAvailable(). Yes, I think that's it.

Wed, Apr 26, 1:35 PM
efriedma accepted D32552: [LibCallsShrinkWrap] Teach the pass how to preserve the dominator.

Could you preserve the DT without requiring it? Not that it matters much for performance, but it would more clear why this pass is requesting the domtree.

Wed, Apr 26, 1:24 PM
efriedma added inline comments to D32552: [LibCallsShrinkWrap] Teach the pass how to preserve the dominator.
Wed, Apr 26, 12:58 PM
efriedma created D32551: [GlobalOpt] Correctly update metadata when localizing a global..
Wed, Apr 26, 12:17 PM
efriedma added a comment to D32549: Add AAMDNodes to memcpy loads/stores during memcpy SDAG expansion.

I don't think you can mark aliasing like that;. see https://bugs.llvm.org/show_bug.cgi?id=11763 .

Wed, Apr 26, 11:25 AM
efriedma added inline comments to D32281: [ARM] ACLE Chapter 9 intrinsics.
Wed, Apr 26, 11:05 AM
efriedma accepted D31944: [DAGCombiner] add (sext i1 X), 1 --> zext (not i1 X).

I think you've covered the interesting cases; LGTM.

Wed, Apr 26, 10:49 AM

Tue, Apr 25

efriedma accepted D32505: [TargetLowering] fix isConstTrueVal to account for build vector truncation.

Sure. LGTM.

Tue, Apr 25, 6:25 PM
efriedma added a comment to D32505: [TargetLowering] fix isConstTrueVal to account for build vector truncation.

I was hoping that ISD::isConstantSplatVector() would have worked here, but it doesn't

Tue, Apr 25, 3:11 PM
efriedma updated subscribers of D32505: [TargetLowering] fix isConstTrueVal to account for build vector truncation.
Tue, Apr 25, 3:08 PM
efriedma added a comment to D32456: [ubsan] Make the alignment check work with some extern decls (PR32630).

This should work correctly with alignment attributes, I think? IIRC those just change the return value of getDeclAlign().

Tue, Apr 25, 2:13 PM
efriedma added a comment to D32456: [ubsan] Make the alignment check work with some extern decls (PR32630).

Err, that's not what I meant...

Tue, Apr 25, 1:26 PM
efriedma added a comment to D32456: [ubsan] Make the alignment check work with some extern decls (PR32630).

If the alignment isn't explicitly set, it is initialized to 0.

Tue, Apr 25, 12:20 PM
efriedma added a comment to D31944: [DAGCombiner] add (sext i1 X), 1 --> zext (not i1 X).

I'm not sure your description is the full story about the ARM code for cmpgt_sext_inc_vec; it looks like the following gets simplified for AVX2 (might want to include this as a testcase):

Tue, Apr 25, 12:19 PM
efriedma added a comment to D32473: [MSP430] Fix PR32769: Select8 and Select16 need to have SR in Uses..

Looks fine, but someone who knows the MSP430 backend better should look at this.

Tue, Apr 25, 11:32 AM
efriedma added a reviewer for D32473: [MSP430] Fix PR32769: Select8 and Select16 need to have SR in Uses.: asl.
Tue, Apr 25, 11:30 AM
efriedma added a comment to D32483: [EarlyCSE] Make branches unconditional if the condition is known.

EarlyCSE is supposed to preserve the CFG (calls setPreservesCFG, depends on DomTree/MemorySSA etc.), so you can't replace a conditional branch with an unconditional branch. Replacing the value of the condition with "true" or "false" is fine, though.

Tue, Apr 25, 10:54 AM

Mon, Apr 24

efriedma added a comment to D32456: [ubsan] Make the alignment check work with some extern decls (PR32630).

Note: it would still not be able to catch the following case --

Mon, Apr 24, 5:20 PM
efriedma added inline comments to D32391: [SelectionDAG] Improve support for promotion of <1 x fX> floating point argument types (PR31088).
Mon, Apr 24, 3:33 PM
efriedma added a comment to D30759: With PIE on x86_64, keep hot local arrays on the stack.

There's also another possibility: you could make the register allocator prefer to spill some other register which isn't on the critical path. Not sure if that's practical for the loops you care about.

Mon, Apr 24, 3:15 PM
efriedma accepted D31856: Headers: Make the type of SIZE_MAX the same as size_t.

LGTM.

Mon, Apr 24, 12:42 PM
efriedma added inline comments to D32281: [ARM] ACLE Chapter 9 intrinsics.
Mon, Apr 24, 12:37 PM
efriedma added inline comments to D32433: Compute safety information in a much finer granularity..
Mon, Apr 24, 12:20 PM
efriedma accepted D32261: [LoopUnroll] Don't try to unroll non-rotated loops.

LGTM.

Mon, Apr 24, 12:02 PM

Apr 21 2017

efriedma added inline comments to D31856: Headers: Make the type of SIZE_MAX the same as size_t.
Apr 21 2017, 6:38 PM