Page MenuHomePhabricator

rnk (Reid Kleckner)
User

Projects

User Details

User Since
Jan 2 2013, 4:34 PM (314 w, 6 d)

Recent Activity

Yesterday

rnk accepted D56747: [EH] Rename llvm.x86.seh.recoverfp intrinsic to llvm.eh.recoverfp.

Looks good to me with a minor comment fix. Let's wait for @efriedma to approve the new name before we commit to it, though.

Tue, Jan 15, 3:30 PM
rnk added a comment to D56329: Fix some warnings on MSVC.

Non gmock changes look good.

Tue, Jan 15, 3:26 PM
rnk committed rL351249: [clang-cl] Alias /Zc:alignedNew[-] to -f[no-]aligned-allocation.
[clang-cl] Alias /Zc:alignedNew[-] to -f[no-]aligned-allocation
Tue, Jan 15, 1:29 PM
rnk committed rC351249: [clang-cl] Alias /Zc:alignedNew[-] to -f[no-]aligned-allocation.
[clang-cl] Alias /Zc:alignedNew[-] to -f[no-]aligned-allocation
Tue, Jan 15, 1:28 PM
rnk accepted D56516: [SanitizerCoverage] Don't create comdat for interposable functions..

In my local tests, the weak function and its sancov metadata both get discarded with bfd, gold, and lld.

Tue, Jan 15, 1:15 PM
rnk updated subscribers of D51664: [IR] Lazily number instructions for local dominance queries.

After giving you a hard time about this a few months ago, I've come around to believing that this is the right thing to do. Certain classes of algorithms really do benefit from having a lexicographic ordering comparison that is fast, and I think that this general approach is the best way to go.

Tue, Jan 15, 11:59 AM
rnk added a comment to D53541: [COFF, ARM64] Do not emit x86_seh_recoverfp intrinsic.

@rnk Thanks a lot for the clarification. Yes, I see o.x: 0 instead of 42. Supporting this case would mean implementing recoverfp as well as support generating the correct parent frame offset for arm64 windows. Do you think this can be done in a follow-up patch? So this patch and D53540 would add the basic support for SEH and we can go fix corner/more complex cases in follow-up patches. There are also several more comprehensive test cases in https://github.com/Microsoft/windows_seh_tests which we plan to address next.

Tue, Jan 15, 11:46 AM
rnk added a comment to D55293: [LLD][COFF] Unmerged sections.

Hi @ruiu ! This is waiting for your final review / approval. Could you possibly take a quick look please? Thank you!

Tue, Jan 15, 11:34 AM · lld
rnk updated subscribers of D56516: [SanitizerCoverage] Don't create comdat for interposable functions..
  • For ELF targets, this fix is functionally correct, but it will have the effect of retaining the section content for the weak symbols in the output, potentially slowing the link and making the output larger (unless you use -gc-sections).

Yes, but this is definitely preferred over breaking weak/strong semantics for our users.

Tue, Jan 15, 11:24 AM
rnk added a comment to D53541: [COFF, ARM64] Do not emit x86_seh_recoverfp intrinsic.

I can't compile the example you gave yet because I haven't applied your patches locally, but this is the "three stack pointer" case that I have in mind:

struct Foo {
  void (*ptr)();
  int x, y, z;
};
Tue, Jan 15, 11:01 AM

Mon, Jan 14

rnk abandoned D55781: Make CC mangling conditional on the ABI version.

Okay. In my opinion, then, we should just do this unconditionally.

Mon, Jan 14, 6:08 PM
rnk accepted D56548: Fix emission of _fltused for MSVC..

I have no idea how you ended up here, but looks good, just a few NFC comments. :)

Mon, Jan 14, 6:03 PM
rnk added a comment to D56473: Allow 'static' storage specifier on an out-of-line member function template declaration in MSVCCompat mode.

Functionality seems reasonable, I'll let @aaron.ballman finish the review.

Mon, Jan 14, 5:51 PM
rnk accepted D56686: [X86] Make _xgetbv/_xsetbv on non-windows platforms.

lgtm

Mon, Jan 14, 5:36 PM
rnk committed rL351146: [X86] Avoid clobbering ESP/RSP in the epilogue..
[X86] Avoid clobbering ESP/RSP in the epilogue.
Mon, Jan 14, 5:29 PM
rnk closed D56617: [X86] Avoid clobbering ESP/RSP in the epilogue..
Mon, Jan 14, 5:29 PM
rnk added a comment to D55585: RFC: [LLD][COFF] Parallel GHASH generation at link-time.

To the light of all this, does it still makes sense to compute and emit GHASH streams in clang? I'm pretty sure that the cost for serialization and I/O for those streams would be much higher that just computing GHASHes on-the-fly in the LLD. The only marginal benefit would be for incrementally linking, however even that is debatable.

Mon, Jan 14, 4:36 PM
rnk added inline comments to D56686: [X86] Make _xgetbv/_xsetbv on non-windows platforms.
Mon, Jan 14, 4:18 PM
rnk updated the diff for D51664: [IR] Lazily number instructions for local dominance queries.
  • rebase
Mon, Jan 14, 3:49 PM
rnk added a comment to D53541: [COFF, ARM64] Do not emit x86_seh_recoverfp intrinsic.

What about the three stack pointer case of stack realignment plus a dynamic alloca? Typically this is the case where recoverfp is necessary.

@rnk Sorry, I missed your comment earlier. I am not sure what scenario the three stack pointer case is. Could you please give me a test case for it?

Mon, Jan 14, 2:18 PM
rnk accepted D56481: [llvm-objcopy] [COFF] Implement --strip-all[-gnu] for symbols.

lgtm

Mon, Jan 14, 2:11 PM
rnk added inline comments to D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.
Mon, Jan 14, 2:09 PM
rnk accepted D56617: [X86] Avoid clobbering ESP/RSP in the epilogue..

Thanks for the report and patch! Looks good. Do you need someone to commit this?

Mon, Jan 14, 1:41 PM
rnk accepted D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.

Thanks for pushing the MC work to support this forward!

Mon, Jan 14, 1:35 PM
rnk accepted D56463: [SEH] Pass the frame pointer from SEH finally to finally functions.

Thanks! I'm surprised we passed as much of the Microsoft exception tests as we did with this bug. Maybe it regressed.

Mon, Jan 14, 1:14 PM
rnk accepted D56671: [COFF, ARM64] Add __nop intrinsic.

lgtm

Mon, Jan 14, 12:53 PM

Fri, Jan 4

rnk updated the diff for D51664: [IR] Lazily number instructions for local dominance queries.

rebase

Fri, Jan 4, 3:37 PM
rnk added a comment to D56275: x86 interrupt calling convention: Fix argument offsets.

@phil-opp, sorry for the breakage, I understand your frustration. From my perspective, the x86 interrupt calling convention was hacked into the x86 backed in a way that doesn't play nice with orthogonal features, like the copy elision I added. Your fix proliferates the wrong direction of the existing design of the interrupt convention. Yes, it fixes the problem, but if I don't push back, require a test, contact Intel, the original authors of this feature, and pressure them to do it in a more proper way, LLVM will continue to become more unmaintanable.

Fri, Jan 4, 3:15 PM
rnk accepted D56334: [LLD][COFF] Parallel sort publics.

lgtm

Fri, Jan 4, 3:03 PM

Thu, Jan 3

rnk added a comment to D56037: [AArch64] Emit the correct MCExpr relocations specifiers like VK_ABS_G0, etc.

Still needs tests, right?

Thu, Jan 3, 6:53 PM
rnk accepted D56200: Remove unused %host_cc lit pattern.

lgtm

Thu, Jan 3, 11:21 AM
rnk accepted D56273: Validate -add-plugin arguments..

lgtm!

Thu, Jan 3, 10:16 AM
rnk updated subscribers of D56275: x86 interrupt calling convention: Fix argument offsets.
Thu, Jan 3, 10:10 AM
rnk added a comment to D56245: Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC..

I don't think this should be llvm_unreachable, I think it should be report_fatal_error. LLVM has some some support for out of tree backends, and they could plausibly call this when using a release build of LLVM as a library. If all the callers were in-tree, then calling this would represent an LLVM-internal logic error (forgetting to check hasRawTextSupport()), and unreachable or assert would be appropriate.

Thu, Jan 3, 9:52 AM

Fri, Dec 28

rnk committed rL350132: Speculative fix for xray assembler error on MachO since r349976.
Speculative fix for xray assembler error on MachO since r349976
Fri, Dec 28, 10:56 AM
rnk committed rCRT350132: Speculative fix for xray assembler error on MachO since r349976.
Speculative fix for xray assembler error on MachO since r349976
Fri, Dec 28, 10:56 AM

Thu, Dec 27

rnk added a comment to D54016: [X86] don't allow X86_64 PIC mode addresses to be used as immediates.

Here is the original test case from the PR:

#include <stdio.h>
int offset = 0;
static void foo2() { printf("foo2 is called\n"); }
void foo() {
  __asm__ volatile("movq %0,%%gs:(%1)" : : "ir"((void *)&foo2), "r"(offset));
  printf("foo is called\n");
}

If you compile with GCC, you will see that it generates the same assembly that LLVM does:

foo:
.LFB15:
        .cfi_startproc
        subq    $8, %rsp
        .cfi_def_cfa_offset 16
        movl    offset(%rip), %eax
#APP
# 21 "t.c" 1
        movq $foo2,%gs:(%eax)
# 0 "" 2
#NO_APP
        leaq    .LC1(%rip), %rdi
        call    puts@PLT
        addq    $8, %rsp
        .cfi_def_cfa_offset 8
        ret
        .cfi_endproc
Thu, Dec 27, 11:42 AM
rnk requested changes to D54016: [X86] don't allow X86_64 PIC mode addresses to be used as immediates.

It looks like this was actually intentionally removed back in rL107727 to fix https://llvm.org/pr4752. This change breaks the test that was added for it, llvm/test/CodeGen/X86/2010-07-06-asm-RIP.ll. I guess, since this is GCC inline asm, the question is, how does GCC handle the 'i' constraint? I suspect that this new behavior may be closer to GCC, but it'll require more analysis.

Thu, Dec 27, 11:31 AM
rnk added a comment to D54667: [CodeView] Emit proper debug info for ref-qualified member functions.

In Zig, a struct acts as a namespace full of static functions, so what you said works perfectly.

I'm also open for suggestions if there was a better way to represent this in LLVM's debug info.

Thu, Dec 27, 10:40 AM
rnk added inline comments to D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer.
Thu, Dec 27, 10:34 AM

Wed, Dec 26

rnk added a comment to D54667: [CodeView] Emit proper debug info for ref-qualified member functions.

This shouldn't crash anymore after rL350073.

Wed, Dec 26, 1:56 PM
rnk committed rL350073: [codeview] Check if this 'this' type of a method is a pointer.
[codeview] Check if this 'this' type of a method is a pointer
Wed, Dec 26, 1:55 PM
rnk added a comment to D54667: [CodeView] Emit proper debug info for ref-qualified member functions.

We can change the code to avoid the crash, but what debug info did you want clang to emit? It seems like the best way to describe StructWithNoFields_add is as a static method of the class StructWithNoFields, since it doesn't appear to have a 'this' parameter.

Wed, Dec 26, 1:29 PM
rnk accepted D54016: [X86] don't allow X86_64 PIC mode addresses to be used as immediates.

lgtm, thanks! Do you need someone to commit this?

Wed, Dec 26, 1:11 PM
rnk committed rC350072: [clang-cl] Treat inputs as C++ with /E, like MSVC.
[clang-cl] Treat inputs as C++ with /E, like MSVC
Wed, Dec 26, 1:07 PM
rnk committed rL350072: [clang-cl] Treat inputs as C++ with /E, like MSVC.
[clang-cl] Treat inputs as C++ with /E, like MSVC
Wed, Dec 26, 1:07 PM
rnk committed rL350071: [MS] Mangle return adjusting thunks with the public access specifier.
[MS] Mangle return adjusting thunks with the public access specifier
Wed, Dec 26, 12:12 PM
rnk committed rC350071: [MS] Mangle return adjusting thunks with the public access specifier.
[MS] Mangle return adjusting thunks with the public access specifier
Wed, Dec 26, 12:12 PM
rnk committed rC350068: Ignore ConstantExpr in IgnoreParens.
Ignore ConstantExpr in IgnoreParens
Wed, Dec 26, 9:48 AM
rnk committed rL350068: Ignore ConstantExpr in IgnoreParens.
Ignore ConstantExpr in IgnoreParens
Wed, Dec 26, 9:48 AM
rnk closed D55853: Ignore ConstantExpr in IgnoreParens.
Wed, Dec 26, 9:48 AM
rnk added inline comments to D55853: Ignore ConstantExpr in IgnoreParens.
Wed, Dec 26, 9:47 AM

Fri, Dec 21

rnk added inline comments to D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.
Fri, Dec 21, 4:13 PM
rnk added a comment to D55853: Ignore ConstantExpr in IgnoreParens.

Want to stamp this? It's 4pm on the Friday before Christmas, what could go wrong? :)

Fri, Dec 21, 4:10 PM
rnk committed rL349976: [MC] Enable .file support on COFF and diagnose it on unsupported targets.
[MC] Enable .file support on COFF and diagnose it on unsupported targets
Fri, Dec 21, 3:39 PM
rnk closed D55900: [MC] Enable .file support on COFF and diagnose it on unsupported targets.
Fri, Dec 21, 3:39 PM
rnk added a comment to D55585: RFC: [LLD][COFF] Parallel GHASH generation at link-time.

One more thing I just realized that was probably obvious to you: Computing ghash in parallel completely eliminates the need for the /debug:ghash flag, and we can eliminate the non-ghash codepath. That's great.

Fri, Dec 21, 3:36 PM
rnk added a comment to D55585: RFC: [LLD][COFF] Parallel GHASH generation at link-time.

Neat! I didn't have a chance to review with a fine toothed comb, but I like the idea, and here's how I analyze it. We've already talked about how merging a type record without ghash is O(size of type), and with a content hash, you can make that O(size of hash), i.e. look at 8 or 20 bytes instead of up to 64K. But, computing the hash requires reading the type record data, so we made the compiler do it and put it in .debug$H.
If one can't do that because MSVC is being used, parallelizing the hash lookup makes it so that the linker only takes O(size of all types / n cores) wall time to do it, and then it does O(# types) (not size of types) work to deduplicate them.

Fri, Dec 21, 3:12 PM
rnk accepted D55951: [LLD][COFF] Fix file/line retrieval when a undefined symbol is to be printed.

lgtm

Fri, Dec 21, 1:34 PM
rnk added inline comments to D55951: [LLD][COFF] Fix file/line retrieval when a undefined symbol is to be printed.
Fri, Dec 21, 12:13 PM
rnk committed rL349945: [BasicAA] Fix AA bug on dynamic allocas and stackrestore.
[BasicAA] Fix AA bug on dynamic allocas and stackrestore
Fri, Dec 21, 12:02 PM
rnk closed D55969: [BasicAA] Fix AA bug on dynamic allocas and stackrestore.
Fri, Dec 21, 12:02 PM

Thu, Dec 20

rnk updated the diff for D55969: [BasicAA] Fix AA bug on dynamic allocas and stackrestore.
  • add positive test for dynamic allocas with intervening call
Thu, Dec 20, 5:48 PM
rnk committed rC349872: [mingw] Don't mangle thiscall like fastcall etc.
[mingw] Don't mangle thiscall like fastcall etc
Thu, Dec 20, 5:45 PM
rnk committed rL349873: [memcpyopt] Add debug logs when forwarding memcpy src to dst.
[memcpyopt] Add debug logs when forwarding memcpy src to dst
Thu, Dec 20, 5:45 PM
rnk committed rL349872: [mingw] Don't mangle thiscall like fastcall etc.
[mingw] Don't mangle thiscall like fastcall etc
Thu, Dec 20, 5:45 PM
rnk added a comment to D55969: [BasicAA] Fix AA bug on dynamic allocas and stackrestore.

My experience so far with inalloca has been that, no, those passes are not safe. This is only the latest in a long line of bugs in LLVM around handling dynamic allocas. I think conflating static and dynamic allocas was a mistake, and we would've been better served with a separate construct for allocating stack memory for local variables, but at this stage, we'd be better off standardizing on the terminology of "alloca" for standard locals and pushing dynamic allocas into intrinsic instructions.

Thu, Dec 20, 5:09 PM
rnk created D55969: [BasicAA] Fix AA bug on dynamic allocas and stackrestore.
Thu, Dec 20, 4:17 PM

Wed, Dec 19

rnk added a comment to D55900: [MC] Enable .file support on COFF and diagnose it on unsupported targets.

However, COFF actually does have some support for this directive, and if you emit an object file, llvm-mc does not assert.

I'm not sure I'm following here - if you emit a COFF object file, from what source? From normal LLVM IR CodeGen, or from MC assembly with a .file directive?

Wed, Dec 19, 1:57 PM
rnk retitled D55853: Ignore ConstantExpr in IgnoreParens from Ignore ConstantExpr in IgnoreParenNoopCasts to Ignore ConstantExpr in IgnoreParens.
Wed, Dec 19, 1:53 PM
rnk updated the diff for D55853: Ignore ConstantExpr in IgnoreParens.
  • move to IgnoreParens
Wed, Dec 19, 1:53 PM
rnk added inline comments to D55853: Ignore ConstantExpr in IgnoreParens.
Wed, Dec 19, 1:52 PM
rnk created D55900: [MC] Enable .file support on COFF and diagnose it on unsupported targets.
Wed, Dec 19, 1:29 PM
rnk committed rL349681: [llvm-ar] Simplify string table get-or-insert pattern with .insert, NFC.
[llvm-ar] Simplify string table get-or-insert pattern with .insert, NFC
Wed, Dec 19, 12:57 PM
rnk accepted D55336: [CodeView] Emit global variables within lexical scopes to limit visibility.

Looks good, thanks.

Wed, Dec 19, 11:24 AM
rnk accepted D55627: [ThinLTO] Remove dllimport attribute from locally defined symbols.

lgtm, thanks!

Wed, Dec 19, 9:59 AM

Tue, Dec 18

rnk added inline comments to D55627: [ThinLTO] Remove dllimport attribute from locally defined symbols.
Tue, Dec 18, 3:06 PM
rnk added inline comments to D55853: Ignore ConstantExpr in IgnoreParens.
Tue, Dec 18, 2:59 PM
rnk added inline comments to D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.
Tue, Dec 18, 2:57 PM
rnk created D55853: Ignore ConstantExpr in IgnoreParens.
Tue, Dec 18, 1:56 PM
rnk accepted D54153: Fix compilation issue in VS2017 with Clang-tablegen and LLVM-tablegen.

Go for it.

Tue, Dec 18, 11:27 AM
rnk added inline comments to D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.
Tue, Dec 18, 9:30 AM
rnk accepted D55670: [RFC] [COFF] [AArch64] Avoid crashing on .seh directives in assembly.

lgtm

Tue, Dec 18, 9:18 AM

Mon, Dec 17

rnk added inline comments to D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.
Mon, Dec 17, 6:11 PM
rnk committed rL349436: [COFF] Set the CPU string for LTO like ELF does.
[COFF] Set the CPU string for LTO like ELF does
Mon, Dec 17, 6:03 PM
rnk committed rLLD349436: [COFF] Set the CPU string for LTO like ELF does.
[COFF] Set the CPU string for LTO like ELF does
Mon, Dec 17, 6:02 PM
rnk committed rL349433: [codeview] Update comment on aligning symbol records.
[codeview] Update comment on aligning symbol records
Mon, Dec 17, 5:39 PM
rnk accepted D55748: [AArch64] [MinGW] Allow enabling SEH exceptions.
Mon, Dec 17, 5:23 PM
rnk added a comment to D55748: [AArch64] [MinGW] Allow enabling SEH exceptions.

lgtm I think it's worth making this work incrementally like this.

Mon, Dec 17, 5:23 PM
rnk added a comment to D55293: [LLD][COFF] Unmerged sections.

Is .CRT$XCU just an example, or do you want to do something special for .CRT$XCU? If latter is the case, have you considered just directly implementing what you want do?

Mon, Dec 17, 5:20 PM · lld
rnk committed rLLD349431: [codeview] Align symbol records to save 441MB during linking clang.pdb.
[codeview] Align symbol records to save 441MB during linking clang.pdb
Mon, Dec 17, 5:17 PM
rnk committed rL349431: [codeview] Align symbol records to save 441MB during linking clang.pdb.
[codeview] Align symbol records to save 441MB during linking clang.pdb
Mon, Dec 17, 5:17 PM
rnk added a comment to D55293: [LLD][COFF] Unmerged sections.

@ruiu, the motivation is to have some abstraction over the collection of chunks before "merging" section names. So, this would allow us to enumerate all chunks contributing to the .CRT$XCU section before it is concatenated with all the other .CRT$* sections and then merged into the .rdata section by our builtin /merge: rule. We accumulate those sections in a plain vector currently, and I felt it would be useful to have some name for that grouping.

Mon, Dec 17, 5:06 PM · lld
rnk resigned from D55769: Add support for demangling msvc's noexcept types to llvm-undname.

Oops, too many Zacharies. I mistook you for him (only in this patch, not the other one). He has more ownership over this code, so I will let him approve it.

Mon, Dec 17, 3:43 PM
rnk committed rC349415: Fix ms-layout_version declspec test and add missing new test.
Fix ms-layout_version declspec test and add missing new test
Mon, Dec 17, 3:20 PM
rnk committed rL349415: Fix ms-layout_version declspec test and add missing new test.
Fix ms-layout_version declspec test and add missing new test
Mon, Dec 17, 3:20 PM
rnk committed rC349414: Update Microsoft name mangling scheme for exception specifiers in the type….
Update Microsoft name mangling scheme for exception specifiers in the type…
Mon, Dec 17, 3:14 PM
rnk committed rL349414: Update Microsoft name mangling scheme for exception specifiers in the type….
Update Microsoft name mangling scheme for exception specifiers in the type…
Mon, Dec 17, 3:14 PM
rnk closed D55685: Update Microsoft name mangling scheme for exception specifiers in the type system.
Mon, Dec 17, 3:14 PM
rnk added a comment to D54554: [PDB] Add symbol records in bulk.

Regarding the alignment optimization, I implemented it, and I only measured a 3% speedup when making clang.pdb (without ghash, I think, since I was lazy and didn't rebuild...). However, I put in stats to show that it saves 433MB of memory allocations, and that's a significant fraction of the overall process memory usage, so it's definitely worth it. I'll post a patch with harder numbers soon.

Mon, Dec 17, 3:05 PM