efriedma (Eli Friedman)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 10 2016, 1:07 PM (80 w, 2 d)

Recent Activity

Yesterday

efriedma added a comment to D41949: [RISCV] implement li pseudo instruction.

Missing testcase for "li a0, foo".

Fri, Feb 23, 12:10 PM

Thu, Feb 22

efriedma added a comment to D43643: [RFC] Sceptre a Spectre variant 1 detector.

I think the focus on array bounds checks is overly narrow. You can get an almost identical effect in other ways. For example, if you use C++ inheritance, a member of an object might be a user-controlled integer on one path, and a pointer on a different path. Or if you have an array of pointers, you could read past the end of the array into uninintialized data.

Thu, Feb 22, 5:53 PM
efriedma added inline comments to D43618: [X86][MMX] Add support for MMX build vectors (PR29222).
Thu, Feb 22, 2:34 PM
efriedma added a comment to D43643: [RFC] Sceptre a Spectre variant 1 detector.

Is the pass intentionally named "Sceptre"? That's amusing, but kind of confusing.

Thu, Feb 22, 1:01 PM
efriedma added inline comments to D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection.
Thu, Feb 22, 12:42 PM
efriedma added inline comments to D43618: [X86][MMX] Add support for MMX build vectors (PR29222).
Thu, Feb 22, 12:35 PM
efriedma added a reviewer for D43594: [AMDGPU] Respect pragma unroll when loop contains convergent instructions: jlebar.
Thu, Feb 22, 11:45 AM

Wed, Feb 21

efriedma added inline comments to D43594: [AMDGPU] Respect pragma unroll when loop contains convergent instructions.
Wed, Feb 21, 6:11 PM
efriedma added inline comments to D41932: [RISCV] Hooks for enabling instruction compression.
Wed, Feb 21, 3:15 PM
efriedma added a comment to D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection.

You should be able to do verification in linear time. Just call SelectionDAG::AssignTopologicalOrder() before you start iterating over allnodes().

Wed, Feb 21, 12:42 PM
efriedma added a comment to D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection.

You should be able to do verification in linear time. Just call SelectionDAG::AssignTopologicalOrder() before you start iterating over allnodes().

Wed, Feb 21, 12:31 PM
efriedma added a comment to D43423: [SimplifyCFG] Create flag to disable simplifyCFG..

FWIW, I'd very much prefer the details of the optimizer wouldn't be exposed as frontend flags.

Wed, Feb 21, 12:27 PM
efriedma added a comment to D43565: [SimplifyCFG] Expanding scope for hoisting common instructions after branch .

LLVM has a GVNHoist pass, which is both more efficient and more effective than anything we can do in SimplifyCFG. (It's currently off by default, but it could probably be turned on with a bit more testing/bugfixing.) I would rather focus on getting that turned on, instead of doing something like this.

Wed, Feb 21, 12:06 PM
efriedma added a comment to D43566: [ARM] Fix access to stack arguments when re-aligning SP in Armv6m.

Would it be possible to use ResolveFrameIndexReference here, like ARMBaseRegisterInfo::eliminateFrameIndex does?

Wed, Feb 21, 11:55 AM
efriedma added a comment to D43106: [RISCV] Enable -fforce-enable-int128 through cmake flag COMPILER_RT_HAS_FINT128_FLAG.

RISCV ABI doc specifies the calling convention for __int128_t ("Scalars wider than 2✕XLEN are passed by reference and are replaced in the argument list with the address."). And llc, at least, appears to correctly honor that. So I think we're okay in terms of ABI stability, but someone should verify that clang emits appropriate IR.

Wed, Feb 21, 11:39 AM
efriedma accepted D43549: [ARM] Fix issue with large xor constants..

LGTM

Wed, Feb 21, 11:00 AM

Tue, Feb 20

efriedma added a comment to D43540: [WebAssembly] Enable -Werror=strict-prototypes by default.

If someone is compiling C code that doesn't have undefined behavior, it should work; if it doesn't, that's a clear bug. (As far as I know, there shouldn't be any issues here, but if there are, file a bug and CC me.)

Tue, Feb 20, 5:53 PM
efriedma added inline comments to D43451: [ARM] Mark -1 as cheap in xor's for thumb1.
Tue, Feb 20, 4:05 PM
efriedma added reviewers for D43106: [RISCV] Enable -fforce-enable-int128 through cmake flag COMPILER_RT_HAS_FINT128_FLAG: compnerd, howard.hinnant.
Tue, Feb 20, 3:55 PM
efriedma accepted D43425: [DSE] Don't DSE stores that subsequent memmove calls read from.

LGTM

Tue, Feb 20, 1:59 PM
efriedma accepted D43105: [RISCV] Enable __int128_t and __uint128_t through clang flag.

Please post a follow-up to add this to the 7.0 release notes.

Tue, Feb 20, 1:10 PM

Fri, Feb 16

efriedma added inline comments to D43425: [DSE] Don't DSE stores that subsequent memmove calls read from.
Fri, Feb 16, 6:23 PM
efriedma added inline comments to D43105: [RISCV] Enable __int128_t and __uint128_t through clang flag.
Fri, Feb 16, 5:59 PM
efriedma added a comment to D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection.

I still want a verifier, a function which checks the bits currently saved on SelectionDAG nodes are the same as the bits we would compute from scratch (and calls report_fatal_error() if they aren't). Maybe call it in a couple places in SelectionDAGISel::CodeGenAndEmitDAG() if assertions are enabled.

Fri, Feb 16, 12:28 PM

Thu, Feb 15

efriedma accepted D38128: Handle COPYs of physregs better (regalloc hints).

ARM test changes LGTM

Thu, Feb 15, 6:08 PM
efriedma accepted D43282: [LegalizeDAG] Fix legalization of SETCC.

LGTM

Thu, Feb 15, 5:40 PM
efriedma added a comment to D41104: Set the NoRecurse attribute for the dbg intrinsics..

Not sure it needs any refactoring? I'm more concerned about the "if (F->isIntrinsic()) return false;" part.

Thu, Feb 15, 3:45 PM

Wed, Feb 14

efriedma added a comment to D42846: [PartialInlining] Use isInlineViable to detect constructs preventing inlining..

It would be good to clean them up to make it clear exactly how they differ, yes.

Wed, Feb 14, 3:14 PM
efriedma added inline comments to D43282: [LegalizeDAG] Fix legalization of SETCC.
Wed, Feb 14, 2:15 PM
efriedma added a comment to D42846: [PartialInlining] Use isInlineViable to detect constructs preventing inlining..

Can you explain the difference between llvm::isInlineViable and CodeExtractor::isBlockValidForExtraction? I'm kind of confused why they're checking different things.

Wed, Feb 14, 2:11 PM
efriedma added a comment to D43293: [SimplifyCFG] Don't remove preheaders when we need canonical loops..

You might need to update the BackEdges set in more places, if we can introduce backedges in certain cases.

Wed, Feb 14, 11:55 AM

Tue, Feb 13

efriedma added inline comments to D43254: Use LLVM's DenseSet insteead of unordered_set..
Tue, Feb 13, 1:48 PM
efriedma added inline comments to D43254: Use LLVM's DenseSet insteead of unordered_set..
Tue, Feb 13, 1:37 PM

Mon, Feb 12

efriedma added a comment to D42485: InstSimplify: If divisor element is undef simplify to undef.

udiv etc. have undefined behavior if any element of the divisor is undef. Even without optimizations, it could raise SIGFPE on x86. (See also D41944.)

Mon, Feb 12, 4:55 PM
efriedma accepted D42871: [InstCombine] Simplify MemTransferInst's source and dest alignments separately.

LGTM

Mon, Feb 12, 2:37 PM
efriedma added inline comments to D43176: [LoopInterchange] Incrementally update the dominator tree..
Mon, Feb 12, 1:00 PM
efriedma added inline comments to D42871: [InstCombine] Simplify MemTransferInst's source and dest alignments separately.
Mon, Feb 12, 12:44 PM
efriedma added inline comments to D43106: [RISCV] Enable -fforce-enable-int128 through cmake flag COMPILER_RT_HAS_FINT128_FLAG.
Mon, Feb 12, 12:22 PM
efriedma added inline comments to D42871: [InstCombine] Simplify MemTransferInst's source and dest alignments separately.
Mon, Feb 12, 12:17 PM
efriedma added inline comments to D43176: [LoopInterchange] Incrementally update the dominator tree..
Mon, Feb 12, 12:10 PM
efriedma accepted D43141: [DAG] make binops with undef operands consistent with IR.

LGTM

Mon, Feb 12, 11:44 AM
efriedma added inline comments to D42759: [CGP] Split large data structres to sink more GEPs.
Mon, Feb 12, 11:40 AM
efriedma added a comment to D42846: [PartialInlining] Use isInlineViable to detect constructs preventing inlining..

isInlineViable doesn't care whether the code is actually reachable, so you could pessimize partial inlining in certain cases, e.g. a function which calls va_start conditionally. I don't think we need fix that now, but it would be nice to at least have a testcase demonstrating the issue.

Mon, Feb 12, 11:34 AM

Fri, Feb 9

efriedma added a comment to D43141: [DAG] make binops with undef operands consistent with IR.

Some of the test changes are a little iffy... we might want to adjust the to stop using undef rather than just change the expected result, to try to preserve some of the original intent.

Fri, Feb 9, 4:38 PM
efriedma added a comment to D43105: [RISCV] Enable __int128_t and __uint128_t through clang flag.

So you want int128_t for compiler-rt itself, so you can use the soft-float implementation, but you want to make int128_t opt-in to avoid the possibility of someone getting a link error trying to link code built with clang against libgcc.a? That seems a little convoluted, but I guess it's okay.

Fri, Feb 9, 11:42 AM

Thu, Feb 8

efriedma added a comment to D43105: [RISCV] Enable __int128_t and __uint128_t through clang flag.

This flag can then be used to build compiler-rt for RISCV32.

Thu, Feb 8, 6:07 PM
efriedma added a comment to D42898: Do not spill CSR to stack on entry to noreturn functions.

I did think about that, but came to the conclusion that longjmp would have to restore all callee-saves to the state they had at setjmp, and that setjmp would have to store them into the jmp_buf.

Thu, Feb 8, 1:19 PM
efriedma added a comment to D42898: Do not spill CSR to stack on entry to noreturn functions.

Unfortunately there's still a hole in the safety analysis. Specifically, "nounwind" does not mean what you want it to mean. Consider the following testcase:

Thu, Feb 8, 12:54 PM
efriedma added a comment to D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection.

If we're going to include the "divergent" bit in SDNodes, so we can query it all the time, the bit needs to be correct all the time. The goal of a verifier is to ensure that at any given point, the bits stored in the SelectionDAG are the same as the bits we would compute from scratch. So code still needs to do the right thing to update the divergence bits, if necessary, but the verifier lets us catch mistakes early. This is similar to the way we have a domtree verifier, to ensure transforms correctly update the domtree.

Thu, Feb 8, 12:28 PM

Tue, Feb 6

efriedma added a comment to D42989: [X86] When doing callee save/restore for k-registers make sure we don't use KMOVQ on non-BWI targets.

Does the intel_ocl_bicc calling convention vary depending on whether the CPU has BWI? If it does, we need more tests, to show code generation works correctly when BWI is enabled.

Tue, Feb 6, 5:38 PM
efriedma committed rL324424: Place undefined globals in .bss instead of .data.
Place undefined globals in .bss instead of .data
Tue, Feb 6, 3:25 PM
efriedma closed D41705: Place undefined globals in .bss instead of .data.
Tue, Feb 6, 3:24 PM
efriedma committed rL324422: [LivePhysRegs] Fix handling of return instructions..
[LivePhysRegs] Fix handling of return instructions.
Tue, Feb 6, 3:02 PM
efriedma closed D42655: [LivePhysRegs] Fix handling of return instructions..
Tue, Feb 6, 3:02 PM
efriedma accepted D41705: Place undefined globals in .bss instead of .data.

How should I credit you in the commit message?

Tue, Feb 6, 2:43 PM
efriedma added a comment to D42962: [ARM] Allow 64- and 128-bit types with 't' inline asm constraint.

This behaviour still differs from that of GCC but I think it is actually more correct, since LLVM picks up the right register type based on the datatype of x, while GCC would need an extra operand modifier to achieve the same result

Tue, Feb 6, 12:42 PM
efriedma added a comment to D42951: [CGP] Strength reduce cmp (xor (a, -1), xor(b, -1)) => cmp (b, a).

higher register pressure

Tue, Feb 6, 12:17 PM
efriedma accepted D42737: [LegalizeDAG] Truncate condition operand of ISD::SELECT.
Tue, Feb 6, 11:51 AM

Mon, Feb 5

efriedma added a comment to D42898: Do not spill CSR to stack on entry to noreturn functions.

It looks like this transform destroys stack traces on non-x86 platforms. That technically isn't wrong, I guess, but it seems unfriendly (for example, LLVM produces a stack trace on an assertion failure).

Mon, Feb 5, 3:21 PM
efriedma added a comment to D42889: [LoopIdiomRecognize] Add support for memmove. Fixes PR25165.

This transform isn't legal. Consider:

Mon, Feb 5, 2:18 PM
efriedma accepted D42788: [SDAG] Legalize all CondCodes by inverting them and/or swapping operands.

LGTM

Mon, Feb 5, 1:26 PM
efriedma added inline comments to D42691: [SimplifyCFG] Relax restriction for folding unconditional branches.
Mon, Feb 5, 12:11 PM
efriedma added inline comments to D42788: [SDAG] Legalize all CondCodes by inverting them and/or swapping operands.
Mon, Feb 5, 12:00 PM
efriedma added a comment to D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection.

I'd like to see a verifier somewhere that the divergence bit is still correct after DAGCombine (it could be different from what SelectionDAG::createOperands would compute given how ReplaceAllUsesWith works).

Mon, Feb 5, 11:47 AM

Fri, Feb 2

efriedma updated subscribers of D42691: [SimplifyCFG] Relax restriction for folding unconditional branches.
Fri, Feb 2, 11:26 AM

Thu, Feb 1

efriedma added a comment to D42806: [AArch64] optionally filter out denorms when using frsqrte to calculate sqrt.

Should still change the select operand to FPZero to produce the 'bic' rather than 'bsl' though?

Thu, Feb 1, 11:29 AM
efriedma accepted D42786: Consider endianness in TargetLowering::scalarizeVectorStore().

LGTM

Thu, Feb 1, 11:24 AM
efriedma added a comment to D42806: [AArch64] optionally filter out denorms when using frsqrte to calculate sqrt.

frsqrte produces a finite estimate for denormal numbers. From the ARM ARM:

Thu, Feb 1, 11:01 AM

Wed, Jan 31

efriedma added inline comments to D42574: [ARM] Lower lower saturate to 0 and lower saturate to -1 using bit-operations.
Wed, Jan 31, 3:30 PM
efriedma committed rL323910: [GlobalOpt] Fix exponential compile-time with selects..
[GlobalOpt] Fix exponential compile-time with selects.
Wed, Jan 31, 12:44 PM
efriedma closed D42451: [GlobalOpt] Fix exponential compile-time with selects..
Wed, Jan 31, 12:44 PM
efriedma added inline comments to D42759: [CGP] Split large data structres to sink more GEPs.
Wed, Jan 31, 12:36 PM
efriedma requested review of D42655: [LivePhysRegs] Fix handling of return instructions..
Wed, Jan 31, 12:08 PM
efriedma updated the diff for D42655: [LivePhysRegs] Fix handling of return instructions..

Mark all callee-saved regs live in return blocks in addLiveOuts/addLiveOutsNoPristines. Clean up the code to make the behavior more obvious. Rebase patch against trunk, since the original patch was reverted. Update summary.

Wed, Jan 31, 12:08 PM
efriedma added a comment to D42100: Fix codegen of stores of vectors with non byte-sized elements..

Could you open a new review request instead of reusing the existing one with a new patch? This is going to get confusing.

Wed, Jan 31, 11:49 AM
efriedma added inline comments to D42655: [LivePhysRegs] Fix handling of return instructions..
Wed, Jan 31, 11:32 AM
efriedma added inline comments to D41029: [JumpTables][PowerPC] Let targets decide which switch instructions are suitable for jump tables.
Wed, Jan 31, 11:18 AM
efriedma added a comment to D42737: [LegalizeDAG] Truncate condition operand of ISD::SELECT.

It seems a little dubious to create a SELECT where the condition isn't either an i1 or a naturally promoted i1... maybe it would make sense to restrict that instead? (And fix the VSELECT expansion to just truncate the condition instead of trying to get fancy.)

Wed, Jan 31, 11:02 AM
efriedma accepted D42668: [Analysis] Disable calls to *_finite and other glibc-only functions on Android..

LGTM

Wed, Jan 31, 10:33 AM

Tue, Jan 30

efriedma added inline comments to D42668: [Analysis] Disable calls to *_finite and other glibc-only functions on Android..
Tue, Jan 30, 6:45 PM
efriedma committed rL323841: Revert r323559 due to EXPENSIVE_CHECKS regression..
Revert r323559 due to EXPENSIVE_CHECKS regression.
Tue, Jan 30, 4:43 PM
efriedma added inline comments to D42668: [Analysis] Disable calls to *_finite and other glibc-only functions on Android..
Tue, Jan 30, 3:38 PM
efriedma added a comment to D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection.

As I understand you are concerned about the mutating the SDNode after it has been created in getNode().

Tue, Jan 30, 3:12 PM
efriedma added a comment to D42509: [LivePhysRegs] Preserve pristine registers in blocks with no successors..

I think that issue will be fixed by https://reviews.llvm.org/D42655.

Tue, Jan 30, 12:51 PM
efriedma added a comment to D42100: Fix codegen of stores of vectors with non byte-sized elements..

We never really formally defined the encoding of vectors, I think. The DataLayout has computed the size in bits of vector types as element_size_in_bits*num_elements for a long time, though (so there's no padding between elements), and we've generally had the rule that the lowest element has the lowest address, since that's generally how vector units work on both big-endian and little-endian targets.

Tue, Jan 30, 12:46 PM
efriedma added a comment to D41761: Introduce llvm.nospeculateload intrinsic.

I hope this is an example of what you were referring to @eli.friedman? Or did I misunderstand?

Tue, Jan 30, 8:59 AM

Mon, Jan 29

efriedma added a comment to D34135: [JumpThreading] Use DT to avoid processing dead blocks.

I think PR36133 could be considered a different issue. LVI currently uses a domtree for certain queries involving llvm.assume; to make that work, the domtree would have to be up-to-date every time we call into LVI. But we could solve that separately from the problem of dead code in LVI.

Mon, Jan 29, 7:52 PM
efriedma added inline comments to D42668: [Analysis] Disable calls to *_finite and other glibc-only functions on Android..
Mon, Jan 29, 4:38 PM
efriedma accepted D42051: [ARM] Allow the scheduler to clone a node with glue to avoid a copy CPSR ↔ GPR..

LGTM

Mon, Jan 29, 3:22 PM
efriedma added a comment to D41104: Set the NoRecurse attribute for the dbg intrinsics..

I don't see anything wrong with the patch, exactly... but we probably need to fix TargetTransformInfo::isLoweredToCall to correctly handle intrinsics before we merge this.

Mon, Jan 29, 2:05 PM
efriedma accepted D42385: [InstSimplify] (X * Y) / Y --> X for relaxed floating-point ops.

LGTM

Mon, Jan 29, 1:40 PM
efriedma created D42655: [LivePhysRegs] Fix handling of return instructions..
Mon, Jan 29, 12:24 PM

Sun, Jan 28

efriedma added a comment to D41761: Introduce llvm.nospeculateload intrinsic.

How can two variant-1 attacks be "different" enough that a speculationsafeload would protect against one but not the other, when both exploit the same load operation

Sorry, wasn't quite clear. There are two speculated loads for a variant-1 attack: the load that reads the secret, and the load that leaks the secret to the user. The first load has to be speculation-safe to stop the attack; whether the second is speculation-safe is irrelevant, at least in the proposals so far. That isn't really a fundamental problem, just an illustration that reasoning about these attacks is tricky.

Sun, Jan 28, 1:56 PM

Fri, Jan 26

efriedma added a comment to D41761: Introduce llvm.nospeculateload intrinsic.

I have no idea what form that assurance would take, since I don't know how LangRef handles such matters.

Fri, Jan 26, 9:39 PM
efriedma added a comment to D42451: [GlobalOpt] Fix exponential compile-time with selects..

Though I wonder if it's possible to trigger a similar exponential behavior with GEP instructions

Fri, Jan 26, 5:17 PM
efriedma added a comment to D41761: Introduce llvm.nospeculateload intrinsic.

Do we need to explicitly prohibit it in LangRef so that future transformations don't start understanding too much about what speculationsafeload does?

Fri, Jan 26, 4:00 PM
efriedma committed rL323559: [LivePhysRegs] Preserve pristine regs in blocks with no successors..
[LivePhysRegs] Preserve pristine regs in blocks with no successors.
Fri, Jan 26, 12:24 PM
efriedma closed D42509: [LivePhysRegs] Preserve pristine registers in blocks with no successors..
Fri, Jan 26, 12:24 PM
efriedma requested changes to D41705: Place undefined globals in .bss instead of .data.

I was going to apply this, but then I found there's a "make check" failure caused by this change:

Fri, Jan 26, 12:10 PM
efriedma updated subscribers of D42574: [ARM] Lower lower saturate to 0 and lower saturate to -1 using bit-operations.
Fri, Jan 26, 11:34 AM