efriedma (Eli Friedman)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 10 2016, 1:07 PM (105 w, 18 h)

Recent Activity

Yesterday

efriedma added a comment to D50683: [Android] Set NewAlign for 64-bit Android to 8 bytes.

The C++ standard just says "An integer literal of type std::size_t whose value is the alignment guaranteed by a call to operator new(std::size_t) or operator new[](std::size_t)." I would take that in the obvious sense, that any pointer returned by operator new must have alignment __STDCPP_DEFAULT_NEW_ALIGNMENT__.

Wed, Aug 15, 2:13 PM
efriedma added inline comments to D50791: [SelectionDAG] unroll unsupported vector FP ops earlier to avoid libcalls on undef elements (PR38527).
Wed, Aug 15, 12:59 PM
efriedma added inline comments to D50791: [SelectionDAG] unroll unsupported vector FP ops earlier to avoid libcalls on undef elements (PR38527).
Wed, Aug 15, 12:22 PM
efriedma added inline comments to D49531: [PowerPC] Enhance the selection(ISD::VSELECT) of vector type.
Wed, Aug 15, 12:08 PM

Tue, Aug 14

efriedma committed rL339734: [ARM] Make PerformSHLSimplify add nodes to the DAG worklist correctly..
[ARM] Make PerformSHLSimplify add nodes to the DAG worklist correctly.
Tue, Aug 14, 3:11 PM
efriedma closed D50667: [ARM] Make PerformSHLSimplify add nodes to the DAG worklist correctly..
Tue, Aug 14, 3:11 PM
efriedma added inline comments to D50634: [RISCV] Add support for local PIC addressing.
Tue, Aug 14, 12:28 PM
efriedma accepted D50683: [Android] Set NewAlign for 64-bit Android to 8 bytes.

LGTM

Tue, Aug 14, 11:11 AM

Mon, Aug 13

efriedma added a comment to D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..

LGTM, but I'd feel more comfortable if a second set of eyes looked at this.

Mon, Aug 13, 5:01 PM
efriedma accepted D50310: Improve the legalisation lowering of UMULO.

LGTM. (Do you want me to commit this for you?)

Mon, Aug 13, 4:57 PM
efriedma created D50667: [ARM] Make PerformSHLSimplify add nodes to the DAG worklist correctly..
Mon, Aug 13, 2:45 PM
efriedma added inline comments to D50634: [RISCV] Add support for local PIC addressing.
Mon, Aug 13, 12:09 PM

Fri, Aug 10

efriedma added a comment to D50578: [DAGCombiner][Mips] Don't combine bitcast+store after LegalOperations when the store is volatile, if the resulting store isn't Legal.

Code change LGTM, but someone familiar with the MIPS backend should review the test changes.

Fri, Aug 10, 4:19 PM
efriedma committed rL339479: Fix unused lambda capture warning from r339472..
Fix unused lambda capture warning from r339472.
Fri, Aug 10, 3:03 PM
efriedma added a comment to D50524: [Hexagon] Replace fatal error with remark in HexagonISelLowering.

It might be worth discussing on llvmdev the question of whether it makes sense to add a flag to clang to control general undefined-behavior diagnostics, as a sort of best-effort thing when the optimizer finds it. We have lib/Analysis/Lint.cpp, but there isn't any clang flag to turn it on. Undefined behavior diagnostics are not something that would ever be reliable in the sense of consistently printing the same diagnostics across compiler versions/optimization levels, and it would always have false positives, so I don't think we would ever turn it on by default. But it could be useful to help users figure out "why did my code disappear" when the compiler generates something unexpected.

Fri, Aug 10, 2:56 PM
efriedma committed rL339472: [ARM] Adjust AND immediates to make them cheaper to select..
[ARM] Adjust AND immediates to make them cheaper to select.
Fri, Aug 10, 2:22 PM
efriedma closed D50030: [ARM] Adjust AND immediates to make them cheaper to select..
Fri, Aug 10, 2:22 PM
efriedma added a comment to D50524: [Hexagon] Replace fatal error with remark in HexagonISelLowering.

Example of what I mean; build with clang --target=hexagon -S -O2:

Fri, Aug 10, 1:29 PM
efriedma added a comment to D50524: [Hexagon] Replace fatal error with remark in HexagonISelLowering.

There's nothing wrong with simplifying code that you've proven has undefined behavior; we do that all over the place in LLVM. We already have code that specifically detects invalid loads/stores in instcombine (although it currently doesn't try to check alignment). That said, the Hexagon backend still has to be prepared for the possibility that some later pass will introduce the sequence "r0 = ##74565; r0 = memw(r0+#0)", because each of those instructions is independently valid. So this is just papering over the underlying compiler crash.

Fri, Aug 10, 1:22 PM
efriedma added inline comments to D50507: [CodeGen][ARM] Coerce FP16 vectors to integer vectors when needed.
Fri, Aug 10, 12:53 PM
efriedma accepted D50323: [GVNHoist] Prune out useless CHI insertions.

LGTM

Fri, Aug 10, 12:11 PM

Thu, Aug 9

efriedma added a comment to D50491: [DAGCombiner][AMDGPU][Mips] Fold bitcast with volatile loads if the resulting load is legal for the target..

I guess we could assume anyone using memory-mapped hardware will only do it with legal types, so reducing the number of operations for illegal types doesn't matter. But please add a comment here (and to the related store transform) explaining that.

Thu, Aug 9, 5:25 PM
efriedma added a comment to D50474: [LV] Vectorize header phis that feed from if-convertable latch phis.

Your patch miscompiles the following:

Thu, Aug 9, 2:00 PM
efriedma accepted D50233: [InstCombine] Transform str(n)cmp to memcmp.

LGTM

Thu, Aug 9, 1:38 PM
efriedma added a comment to D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.

Somewhere around CompilerInvocation::CreateFromArgs is probably appropriate.

Thu, Aug 9, 1:35 PM
efriedma added a comment to D50323: [GVNHoist] Prune out useless CHI insertions.

Seems fine to me.

Thu, Aug 9, 1:31 PM
efriedma added a comment to D50507: [CodeGen][ARM] Coerce FP16 vectors to integer vectors when needed.

Do we need to check for homogeneous aggregates of half vectors somewhere?

Thu, Aug 9, 1:30 PM
efriedma added a comment to D50524: [Hexagon] Replace fatal error with remark in HexagonISelLowering.

Fix the pattern to generate a load with the misaligned address? That already happens. The problem is that we have passes that make changes based on the assumption that the instructions are valid.

Thu, Aug 9, 1:19 PM
efriedma added a comment to D50524: [Hexagon] Replace fatal error with remark in HexagonISelLowering.

Converting the memory operation to an explicit trap seems overly complicated, as opposed to just fixing whatever isel pattern is assuming the immediate is divisible by 4. But I don't know the Hexagon backend that well.

Thu, Aug 9, 12:54 PM
efriedma added inline comments to D50179: [AArch64][ARM] Context sensitive meaning of option "crypto".
Thu, Aug 9, 12:17 PM
efriedma added a comment to D50491: [DAGCombiner][AMDGPU][Mips] Fold bitcast with volatile loads if the resulting load is legal for the target..

For volatile loads/stores, we should probably try to preserve the number and addresses of memory operations. If the source and destination types would lower to equivalent memory operations, like i32 vs. f32 on x86, the transform is fine. If they wouldn't, like f64 vs. i64 on x86-32, the transform is dubious; memory-mapped I/O can respond in strange ways if you don't use the right access width.

Thu, Aug 9, 12:05 PM

Wed, Aug 8

efriedma added a comment to D50310: Improve the legalisation lowering of UMULO.

The computation of %LHS.HI * %RHS.HI is only necessary to compute the overflow bit.

Wed, Aug 8, 1:58 PM
efriedma added a comment to D50405: [Hexagon] Diagnose misaligned absolute loads and stores.

We generally prefer not to emit diagnostics for undefined behavior from the backend because the code might be dynamically unreachable (and the user might not be able to do anything about it if the code is unreachable because the compiler cloned the code).

Wed, Aug 8, 1:23 PM
efriedma committed rL339283: [ARM] Avoid spilling lr with Thumb1 tail calls..
[ARM] Avoid spilling lr with Thumb1 tail calls.
Wed, Aug 8, 1:03 PM
efriedma closed D49459: [ARM] Avoid spilling lr with Thumb1 tail calls..
Wed, Aug 8, 1:03 PM
efriedma added a comment to D50310: Improve the legalisation lowering of UMULO.

Please choose a different name for the tests; "muloti2.ll" isn't usefully indicating what the files actually test.

Wed, Aug 8, 12:59 PM
efriedma added a comment to D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature.

Oh, oops, I should have spotted that. No, the point of this was to separate the timing infrastructure, not to change the behavior of -ftime-report.

Wed, Aug 8, 11:58 AM
efriedma added inline comments to D50323: [GVNHoist] Prune out useless CHI insertions.
Wed, Aug 8, 11:40 AM

Tue, Aug 7

efriedma updated the diff for D49465: [ARM] Enable tail calls for all Thumb1 targets..

Updated to use a pass after isel to convert a call in tail position to a tail-call, rather than try to do the opposite in frame lowering. I'm not completely happy with this; the pass is a lot more code than I'd really like. But I don't have any better ideas.

Tue, Aug 7, 5:07 PM
efriedma added a comment to D49994: Allow constraining virtual register's class within reason.

I'd like an explanation for why the generated code is changing for AArch64... generating extra copies clearly seems like a downside. And there isn't any obvious reason for this change to impact register allocation: on AArch64, all i32 register classes contain exactly the same set of allocatable registers.

Tue, Aug 7, 12:44 PM
efriedma added a comment to D49987: [ARM] Make FP16 vectors legal.

We should handle f16 and vectors of f16 in a consistent manner, for the sake of maintaining the code in the future. (Either handle both in the backend, or handle both in clang.)

Tue, Aug 7, 11:54 AM
efriedma accepted D45437: Support inline asm with multiple 64bit output in 32bit GPR.

LGTM

Tue, Aug 7, 11:23 AM
efriedma accepted D50238: [ARM] FP16: support the vector vmin and vmax variants.

LGTM

Tue, Aug 7, 11:20 AM
efriedma accepted D50143: [SLP] Fix insert point for reused extract instructions..

LGTM

Tue, Aug 7, 11:17 AM
efriedma accepted D49727: [CodeGen] emit inline asm clobber list warnings for reserved.

LGTM

Tue, Aug 7, 11:16 AM

Mon, Aug 6

efriedma added a comment to D50143: [SLP] Fix insert point for reused extract instructions..

This version makes a lot more sense. :)

Mon, Aug 6, 5:56 PM
efriedma added a comment to D50238: [ARM] FP16: support the vector vmin and vmax variants.

SelectionDAGBuilder::visitSelect can also generate FMINNAN etc.; please add testcases.

Mon, Aug 6, 5:53 PM
efriedma updated subscribers of D50233: [InstCombine] Transform str(n)cmp to memcmp.
Mon, Aug 6, 5:45 PM
efriedma added inline comments to D49727: [CodeGen] emit inline asm clobber list warnings for reserved.
Mon, Aug 6, 5:32 PM
efriedma accepted D49248: [TargetLowering] Add support for non-uniform vectors to BuildUDIV.

LGTM

Mon, Aug 6, 5:26 PM
efriedma added a comment to D47196: [Time-report ](2): Recursive timers in Clang.

I mean, which of the callers of startFrontendTimer() is calling it with a pointer to std::declval()?

Mon, Aug 6, 5:17 PM
efriedma added a comment to D43269: [MemorySSA] Be less aggressive with @llvm.lifetime.start.

What happened to this patch? It looks like it's a fix for a miscompile?

Mon, Aug 6, 4:13 PM
efriedma added a comment to D50310: Improve the legalisation lowering of UMULO.

I would very much love to add a test that actually executes some machine code with a number of test vectors, but I believe no such test family exists in LLVM.

Mon, Aug 6, 4:11 PM
efriedma accepted D47337: [GVN,NewGVN] Move patchReplacementInstruction to Utils/Local.h.

LGTM

Mon, Aug 6, 12:30 PM
efriedma added a reviewer for D50323: [GVNHoist] Prune out useless CHI insertions: hiraditya.
Mon, Aug 6, 12:24 PM

Fri, Aug 3

efriedma added a comment to D50030: [ARM] Adjust AND immediates to make them cheaper to select..

i16_cmpz specifically is a little weird... TargetLowering::SimplifySetCC has a transform which specifically impacts that case, but only if the immediate isn't legal. Maybe I can come up with something else to exercise the uxtb codepath.

Fri, Aug 3, 4:30 PM
efriedma updated the diff for D50030: [ARM] Adjust AND immediates to make them cheaper to select..

Updated tests.

Fri, Aug 3, 3:37 PM
efriedma committed rC338931: Diagnose invalid cv-qualifiers for friend decls..
Diagnose invalid cv-qualifiers for friend decls.
Fri, Aug 3, 3:10 PM
efriedma committed rL338931: Diagnose invalid cv-qualifiers for friend decls..
Diagnose invalid cv-qualifiers for friend decls.
Fri, Aug 3, 3:10 PM
efriedma closed D45712: Diagnose invalid cv-qualifiers for friend decls..
Fri, Aug 3, 3:10 PM
efriedma added a comment to D45437: Support inline asm with multiple 64bit output in 32bit GPR.

This fix the bug reported in https://reviews.llvm.org/rL337903

Fri, Aug 3, 3:05 PM
efriedma planned changes to D32239: [SCEV] Make SCEV or modeling more aggressive..

getSCEV is still recurses without a limit, so this still blows out the stack, and gets very slow for complex expressions.

Fri, Aug 3, 2:52 PM
efriedma accepted D50113: [SLC] Fix shrinking of pow().

LGTM

Fri, Aug 3, 2:48 PM
efriedma added a comment to D50233: [InstCombine] Transform str(n)cmp to memcmp.

I think you need to add something like "dereferenceable(4)" or "byval" to the parameter. so it's recognized as dereferenceable.

Fri, Aug 3, 2:19 PM
efriedma added inline comments to D50233: [InstCombine] Transform str(n)cmp to memcmp.
Fri, Aug 3, 1:03 PM
efriedma added inline comments to D49248: [TargetLowering] Add support for non-uniform vectors to BuildUDIV.
Fri, Aug 3, 12:52 PM
efriedma added a comment to D50233: [InstCombine] Transform str(n)cmp to memcmp.

Please fix the tests so they don't strcmp against uninitialized memory. Probably the simplest way to do that is use an argument instead of an alloca.

Fri, Aug 3, 12:23 PM
efriedma added a comment to D50223: [Support] Don't initialize compressed buffer allocated by zlib::compress.

SmallVector has a method set_size to change the size of the vector without initializing the elements; you don't need to switch to unique_ptr just for that.

Fri, Aug 3, 12:01 PM

Thu, Aug 2

efriedma committed rL338791: [GlobalMerge] Allow merging globals with explicit section markings..
[GlobalMerge] Allow merging globals with explicit section markings.
Thu, Aug 2, 4:54 PM
efriedma closed D49822: [GlobalMerge] Allow merging globals with explicit section markings..
Thu, Aug 2, 4:54 PM
efriedma added a comment to D47196: [Time-report ](2): Recursive timers in Clang.

"0.0040" is four milliseconds? You're probably crediting time incorrectly, somehow. Can you tell which FrontendTimeRAII the time is coming from?

Thu, Aug 2, 4:37 PM
efriedma added a comment to D49987: [ARM] Make FP16 vectors legal.

Needs tests for hardfloat ABI, in addition to soft-float.

Thu, Aug 2, 4:21 PM
efriedma added a comment to D50079: [ARM] arm.codegen.zeroext intrinsics.

Add to BitCode/compatibility-6.0.ll test and just keep this codegen one too?

Thu, Aug 2, 3:58 PM
efriedma added a comment to D50166: [ARM64] [Windows] MCLayer support for exception handling.

Perhaps I can create a MIR test case with the SEH opcodes, and use llvm-objdump to check the .xdata section?

Thu, Aug 2, 3:20 PM
efriedma added a comment to D50113: [SLC] Fix shrinking of pow().

So, why was it added?

Thu, Aug 2, 3:02 PM
efriedma accepted D50185: [TargetLowering] Generalise BuildSDIV function.

LGTM

Thu, Aug 2, 2:45 PM
efriedma added a comment to D50113: [SLC] Fix shrinking of pow().

I'd like to get this right... both the CheckRetType check, and the "infinite loop" check from optimizeUnaryDoubleFP are probably relevant. Can we share code between the two functions?

Thu, Aug 2, 2:23 PM
efriedma accepted D50191: [ARM] FP16: support VFMA.

LGTM

Thu, Aug 2, 2:18 PM
efriedma added inline comments to D50179: [AArch64][ARM] Context sensitive meaning of option "crypto".
Thu, Aug 2, 11:21 AM

Tue, Jul 31

efriedma added a comment to D50079: [ARM] arm.codegen.zeroext intrinsics.

These clearly shouldn't be target-specific.

Tue, Jul 31, 7:47 PM
efriedma committed rL338465: [MachineOutliner] Clean up subtarget handling..
[MachineOutliner] Clean up subtarget handling.
Tue, Jul 31, 5:37 PM
efriedma closed D49880: [MachineOutliner] Clean up subtarget handling..
Tue, Jul 31, 5:37 PM
efriedma added inline comments to D50113: [SLC] Fix shrinking of pow().
Tue, Jul 31, 3:45 PM
efriedma added a comment to D49064: DAG: Add helper for creating shifts with correct type.

I'm not really a fan of the LegalTypes boolean; it's just asking for people to pass the wrong value. Can we just check whether the type is legal, or something like that?

Tue, Jul 31, 2:55 PM
efriedma added a comment to D50039: [LibCalls] Added nonnull atribute to libc function args.

Yes, that's the right list of functions. Like the discussion noted, we don't really want to match gcc's behavior here because it breaks legacy code for little practical benefit.

Tue, Jul 31, 2:33 PM
efriedma added inline comments to D49928: [SLP] Fix PR38339: Instruction does not dominate all uses!.
Tue, Jul 31, 1:51 PM
efriedma added inline comments to D49928: [SLP] Fix PR38339: Instruction does not dominate all uses!.
Tue, Jul 31, 1:16 PM
efriedma added inline comments to D49928: [SLP] Fix PR38339: Instruction does not dominate all uses!.
Tue, Jul 31, 1:12 PM
efriedma added a comment to D50039: [LibCalls] Added nonnull atribute to libc function args.

I don't think we can reasonably do this for the functions which take a pointer+size pair; see http://lists.llvm.org/pipermail/cfe-dev/2017-January/052066.html etc. Maybe we can mark specific calls where we know the size is nonzero.

Tue, Jul 31, 12:58 PM
efriedma added inline comments to D50077: [LLD][ELF][ARM] Add support for Armv5 and Armv6 compatible Thunks.
Tue, Jul 31, 12:38 PM
efriedma added a comment to D47196: [Time-report ](2): Recursive timers in Clang.

I think there's something weird happening with your timers if they're showing half a second spent parsing or instantiating std::declval: it doesn't have a definition, anywhere (it's just a placeholder for template metaprogramming tricks).

Tue, Jul 31, 12:19 PM
efriedma added a comment to D49987: [ARM] Make FP16 vectors legal.

Can we make <4 x half> a legal type on all subtargets with NEON, and just mark all the operations "expand" for subtargets which don't support math on it?

Tue, Jul 31, 11:44 AM
efriedma added a comment to D50019: [InstCombine] Expand strcmp(s, "x") to memcmp.

Not sure how often that shows up, but yes, if you can prove the the pointer is dereferenceable, it's okay to transform to memcmp.

Tue, Jul 31, 11:40 AM
efriedma added a comment to D50030: [ARM] Adjust AND immediates to make them cheaper to select..

Sure, I can add a few more tests.

Tue, Jul 31, 11:38 AM
efriedma added a comment to D46822: [RISCV] Add driver for riscv32-unknown-elf baremetal target.

Please don't commit patches on behalf of someone else if the author hasn't specifically requested it. There might be some issue which the author forgot to note on the review.

Tue, Jul 31, 11:25 AM

Mon, Jul 30

efriedma created D50030: [ARM] Adjust AND immediates to make them cheaper to select..
Mon, Jul 30, 5:18 PM
efriedma added a comment to D50018: SystemZ: keep AND masks before SHL i128.

Yes, that's right. (Well, except that ISD::ROTL specifically is defined to wrap in the obvious way.)

Mon, Jul 30, 4:02 PM
efriedma added a comment to D50019: [InstCombine] Expand strcmp(s, "x") to memcmp.

If you want to expand strcmp inline, probably the best way to do that is to extend the existing ExpandMemCmpPass pass.

Mon, Jul 30, 4:00 PM
efriedma added a comment to D50019: [InstCombine] Expand strcmp(s, "x") to memcmp.

Oh, okay, that makes more sense.

Mon, Jul 30, 3:41 PM
efriedma added a comment to D50019: [InstCombine] Expand strcmp(s, "x") to memcmp.

This is not correct: consider, for example, strcmp("xx", "x").

Mon, Jul 30, 3:33 PM