Page MenuHomePhabricator

rnk (Reid Kleckner)
User

Projects

User Details

User Since
Jan 2 2013, 4:34 PM (440 w, 5 d)

Recent Activity

Today

rnk added a comment to D101421: [DebugInfo] Enable CodeView DebugInfo for basic block sections.

Sorry for the delay.

Tue, Jun 15, 11:21 AM · Restricted Project
rnk committed rGe32a92c6fe8e: Remove unnecessary triple from test (authored by JohnTitor).
Remove unnecessary triple from test
Tue, Jun 15, 9:50 AM
rnk closed D68891: Remove unnecessary triple from test.
Tue, Jun 15, 9:50 AM · Restricted Project
rnk accepted D68891: Remove unnecessary triple from test.

lgtm

Tue, Jun 15, 9:49 AM · Restricted Project

Yesterday

rnk added a comment to D103772: [clang-cl] Reenable /Zc:twoPhase by default if targetting MSVC 2017 Update 3 or newer.

If we look back at the original intention of our MSVC compatibility work, we tried to accept as much invalid code as necessary to parse "system headers". System headers were widely interpreted to bethe MSVC STL, ATL, and the Windows SDK. So, even if MSVC defaults to delayed template parsing, if system headers parse with /Zc:twoPhase enabled, maybe we could pick a different default. We'd document the behavior change, of course. Delayed template parsing is an ongoing source of bugs (https://llvm.org/pr50676) and maintenance overhead, so it would be worth diverging from MSVC a bit if possible.

Mon, Jun 14, 12:34 PM · Restricted Project
rnk accepted D104001: [X86] avoid assert with varargs, soft float, and no-implicit-float.

The spill is conditional on %al ...

Mon, Jun 14, 12:27 PM · Restricted Project
rnk added inline comments to rGb9af157fd18d: [ASan/Win] Hide index from compiler to avoid new clang warning.
Mon, Jun 14, 12:26 PM
rnk accepted D104246: X86: balance the frame prologue and epilogue on Win64.

The code change is just adjusting some if conditions and changing indentation, so there's no reason to hold it up with my design questions. Please consider implementing the refactoring suggestion as a follow-up, though.

Mon, Jun 14, 12:04 PM · Restricted Project

Fri, Jun 11

rnk added a comment to D103992: [InstCombine / BuildLibCalls] Add parameter attributes from the prototype..

I think the verifier is doing the right thing, I think it's a bug that @llvm.memcpy is carrying a returned attribute on the first argument. I'm not sure how that is happening, but whatever sets that up seems like a bug.

Fri, Jun 11, 4:45 PM · Restricted Project
rnk accepted D104105: [DebugInfo] Prevent non-determinism when updating DIArgList users of a value.

Thanks for finding this, I think it looks good.

Fri, Jun 11, 4:37 PM · Restricted Project, debug-info
rnk added a comment to D104141: [ASan/Win] Fix failing test after ce44fe199bbf.

I fixed it and it went green, thanks for the fix and the heads up though:
https://lab.llvm.org/buildbot/#/builders/127/builds/12308

Fri, Jun 11, 4:28 PM · Restricted Project
rnk committed rGb9af157fd18d: [ASan/Win] Hide index from compiler to avoid new clang warning (authored by rnk).
[ASan/Win] Hide index from compiler to avoid new clang warning
Fri, Jun 11, 4:12 PM
rnk added a comment to D104141: [ASan/Win] Fix failing test after ce44fe199bbf.

IMO it would be more resiliant to hide the index or the pointer from the compiler, let me try to put together that fix.

Fri, Jun 11, 3:54 PM · Restricted Project

Thu, Jun 10

rnk added a comment to D104061: [LangRef] clarify the meaning of noimplicitfloat.

That's a good point, if this attribute disables vectorization of integer math, the docs should say as much.

Thu, Jun 10, 4:01 PM · Restricted Project
rnk added a comment to D104050: [Profile] Handle invalid profile data.

I agree, it's a choice between 2 and 3. 3 would be consistent with what happens when the existing profraw file is from another binary, right? Maybe do that? It risks losing coverage data, and means the corrupt file is persisted. I guess that's best, since it will cause llvm-profdata merge to fail later.

Thu, Jun 10, 3:55 PM · Restricted Project
rnk accepted D104050: [Profile] Handle invalid profile data.

This fixes an urgent crash bug, so let's go ahead and land it.

Thu, Jun 10, 3:51 PM · Restricted Project
rnk accepted D104061: [LangRef] clarify the meaning of noimplicitfloat.

This matches my understanding of the semantics of noimplicitfloat, but I'd like to get a stamp from one of the other people I added to check my understanding.

Thu, Jun 10, 2:51 PM · Restricted Project
rnk added reviewers for D104061: [LangRef] clarify the meaning of noimplicitfloat: pengfei, avl, RKSimon, craig.topper.
Thu, Jun 10, 2:50 PM · Restricted Project
rnk added a comment to D104050: [Profile] Handle invalid profile data.

Sorry about all the nits, bounds checking is all about attention to detail.

Thu, Jun 10, 2:40 PM · Restricted Project
rnk accepted D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used..

lgtm

Thu, Jun 10, 12:09 PM · Restricted Project
rnk added a comment to D104050: [Profile] Handle invalid profile data.

I think it's worth the time to carefully refactor this code so we don't end up in a similar situation, maybe in the value profile data.

Thu, Jun 10, 11:41 AM · Restricted Project

Wed, Jun 9

rnk accepted D103992: [InstCombine / BuildLibCalls] Add parameter attributes from the prototype..

+1

Wed, Jun 9, 2:56 PM · Restricted Project
rnk added a comment to D103752: [LoopNest] Fix Wdeprecated-copy warnings.

The buildbot went green before your revert has landed.
No idea what's going on, but this change does not seem to be the cause of it.

Wed, Jun 9, 2:54 PM · Restricted Project
rnk added inline comments to D102736: Fix tmp files being left on Windows builds..
Wed, Jun 9, 2:53 PM · Restricted Project, Restricted Project
rnk added a comment to D103947: [libcxx] Fix using the vcruntime ABI with _HAS_EXCEPTIONS=0 defined.

I think we can say that _HAS_EXCEPTIONS=0 is an important configuration. We use it in LLVM, you can grep for it:

$ git grep _HAS_EXCEPTIONS ../llvm
../llvm/cmake/modules/AddLLVM.cmake:      list(APPEND LLVM_COMPILE_DEFINITIONS _HAS_EXCEPTIONS=0)
... others
Wed, Jun 9, 2:02 PM · Restricted Project
rnk updated subscribers of D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used..

Won't this change cause weird effects with LTO, when you're merging multiple TUs' global_ctors arrays before emitting? Won't this end up reversing the order of the files, as well as the order of the functions within a single file? That does not seem likely to be expected, right?

Wed, Jun 9, 9:04 AM · Restricted Project

Tue, Jun 8

rnk updated subscribers of D103717: [InstrProfiling] Make __profd_ unconditionally private for ELF.

We are observing some crashes in coverage, but we haven't root caused them yet: crbug.com/1216811
So I guess we need to keep waiting for the dust to settle. @aeubanks was able to repro the crash there, but it's not deterministic.

Tue, Jun 8, 12:38 PM · Restricted Project
rnk accepted D103257: [ms] [llvm-ml] Disambiguate size directives and variable declarations.

lgtm

Tue, Jun 8, 12:19 PM · Restricted Project
rnk updated subscribers of D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used..

I think we do need to think about globalopt. Consider this example:

extern "C" {
int printf(const char *, ...);
extern int sharedState;
int sharedState = 0;
int init1() {
  asm volatile(""); // block globalopt
  return ++sharedState;
}
int init2() { return ++sharedState; }
int gv1 = init1();
int gv2 = init2();
}
Tue, Jun 8, 9:21 AM · Restricted Project

Mon, Jun 7

rnk added inline comments to D101421: [DebugInfo] Enable CodeView DebugInfo for basic block sections.
Mon, Jun 7, 4:18 PM · Restricted Project
rnk accepted D103837: [clang] Apply MS ABI details on __builtin_ms_va_list on non-windows platforms on x86_64.

lgtm

Mon, Jun 7, 3:47 PM · Restricted Project
rnk updated subscribers of D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used..

+ various .ctors stakeholders + @MaskRay + @mstorsjo

Mon, Jun 7, 3:36 PM · Restricted Project
rnk added reviewers for D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used.: efriedma, rsmith, brooksmoses.
Mon, Jun 7, 3:27 PM · Restricted Project
rnk added inline comments to D103806: [SystemZ][z/OS] Pass OpenFlags when creating tmp files.
Mon, Jun 7, 12:30 PM · Restricted Project, Restricted Project
rnk added inline comments to D103452: [clang] Fix reading long doubles with va_arg on x86_64 mingw.
Mon, Jun 7, 12:24 PM · Restricted Project
rnk accepted D103771: [clang][msvc] Define _HAS_STATIC_RTTI to 0, when compiling with -fno-rtti.

Seems reasonable to me.

Mon, Jun 7, 9:46 AM · Restricted Project
rnk added a comment to D103806: [SystemZ][z/OS] Pass OpenFlags when creating tmp files.

I see, thanks.

Mon, Jun 7, 9:35 AM · Restricted Project, Restricted Project

Fri, Jun 4

rnk added a comment to D103288: [SanCov] Properly set ABI parameter attributes.

OK, right, the issue we were trying to solve is that LLVM was crashing when byval (+inalloca, etc) types mismatched somehow.

Fri, Jun 4, 12:39 PM · Restricted Project
rnk added a comment to D103372: [InstrProfiling] If no value profiling, make data variable private and (for Windows) use one comdat.

I think MachO doesn't have comdats, so we need to leave the __profd_ linkage as it was. I think private linkage also prevents atomization, which inhibits GC / dead stripping. I don't think this change really makes sense for MachO, so you could reasonably reland with that change.

Fri, Jun 4, 11:38 AM · Restricted Project, Restricted Project, Restricted Project

Thu, Jun 3

rnk added a comment to D103288: [SanCov] Properly set ABI parameter attributes.

I don't like the idea of having the IRBuilder look at the call target and implicitly setting attributes, but I am open to revisiting the backend change. Here's a rationale for going back to the old behavior:

  • calling a function with the wrong function prototype is UB
  • calling a function with mismatched ABI attributes is very much like prototype mismatch, so it's UB
  • LLVM can generate whatever code it wants in cases of UB. So, if we have a direct callee, why not have the backend implicitly use the attributes from the callee?
Thu, Jun 3, 4:40 PM · Restricted Project
rnk accepted D103415: [BuildLibCalls] Properly set ABI attributes on arguments.

lgtm

Thu, Jun 3, 3:10 PM · Restricted Project
rnk accepted D103415: [BuildLibCalls] Properly set ABI attributes on arguments.

There's some horrifying index manipulation involving unsigned overflow that I had to avoid by checking if either AttributeList is empty.

Thu, Jun 3, 2:10 PM · Restricted Project
rnk accepted D103372: [InstrProfiling] If no value profiling, make data variable private and (for Windows) use one comdat.

This reduced object file size of base_unittests.exe, compiled with coverage, optimizations, and gmlt debug info by 10%:

#BEFORE
Thu, Jun 3, 11:05 AM · Restricted Project, Restricted Project, Restricted Project
rnk accepted D103452: [clang] Fix reading long doubles with va_arg on x86_64 mingw.

lgtm

Thu, Jun 3, 9:24 AM · Restricted Project

Wed, Jun 2

rnk added a comment to D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used..

For PS4, we use the .ctors scheme, and so the initialization order was suddenly reversed, which was not noticed for a while until the user had a dependency on the previous initialization. And using init_array or not has currently no effect on the order of emission of global_ctors.

Wed, Jun 2, 3:46 PM · Restricted Project
rnk added a comment to D103372: [InstrProfiling] If no value profiling, make data variable private and (for Windows) use one comdat.

Thanks! This is really cool. It will probably help reduce coverage object file size by a lot. Let me see if I can get some numbers for that.

Wed, Jun 2, 2:58 PM · Restricted Project, Restricted Project, Restricted Project
rnk accepted D103012: [LLD] [COFF] Fix autoexport from LTO objects with comdat symbols.

lgtm

Wed, Jun 2, 2:33 PM · Restricted Project
rnk added a comment to D103415: [BuildLibCalls] Properly set ABI attributes on arguments.

You're right, I don't see a way to directly merge two AttributeList objects. I looked at AttrBuilder::merge, but AttrBuilder makes AttributeSet objects. I guess it makes sense to have an AttributeList method to do this.

Wed, Jun 2, 2:28 PM · Restricted Project
rnk added a comment to D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used..

Is there anything preventing us from using the existing priority field to define the order instead of introducing the order among initializers with the same priority? If we go with 1., there would be no way to say that "the order does not matter" right?

Wed, Jun 2, 2:22 PM · Restricted Project
rnk committed rG07c2a912ddf1: Add a .mailmap entry for my two email addresses (authored by rnk).
Add a .mailmap entry for my two email addresses
Wed, Jun 2, 2:14 PM
rnk added a comment to D103415: [BuildLibCalls] Properly set ABI attributes on arguments.

I'd suggest something like:

CI->setAttributes(CI->getCalledFunction()->getAttributes().removeAttributes(Ctx, llvm::AttributeList::FunctionIndex)));
Wed, Jun 2, 2:04 PM · Restricted Project
rnk added inline comments to D101421: [DebugInfo] Enable CodeView DebugInfo for basic block sections.
Wed, Jun 2, 1:59 PM · Restricted Project
rnk added inline comments to D103452: [clang] Fix reading long doubles with va_arg on x86_64 mingw.
Wed, Jun 2, 1:57 PM · Restricted Project
rnk accepted D103355: [InstrProfiling] Delete linkage/visibility toggling for Windows.

lgtm, thanks

Wed, Jun 2, 1:22 PM · Restricted Project, Restricted Project
rnk updated subscribers of D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when .ctors/.dtors are used..

I think this would be an unfortunate code size and startup time regression for Itanium C++ inline variables. The existing code was written under the assumption that vague linkage (GVA_DiscardableODR) implies no initialization ordering, but I think you've found a valid counterexample.

Wed, Jun 2, 11:50 AM · Restricted Project

Sat, May 29

rnk added a comment to D103355: [InstrProfiling] Delete linkage/visibility toggling for Windows.

The main impact of this change seems to be that PGO data for non-comdat code is now private instead of internal. That's a good change: PGO data has really high object file size overhead, and LLD already discards PGO symbol names from the PDB because they are so large.

Sat, May 29, 8:07 AM · Restricted Project, Restricted Project

Fri, May 28

rnk committed rG4c6e2774d827: [gn] Make ubsan errors fatal, as in cmake (authored by rnk).
[gn] Make ubsan errors fatal, as in cmake
Fri, May 28, 8:06 AM
rnk closed D103298: [gn] Make ubsan errors fatal, as in cmake.
Fri, May 28, 8:05 AM · Restricted Project

Thu, May 27

rnk committed rGef4cfd832238: Pass -gcodeview-ghash when using clang-cl and lld-link (authored by rnk).
Pass -gcodeview-ghash when using clang-cl and lld-link
Thu, May 27, 9:02 PM
rnk closed D103287: Pass -gcodeview-ghash when using clang-cl and lld-link.
Thu, May 27, 9:02 PM · Restricted Project
rnk updated the diff for D103287: Pass -gcodeview-ghash when using clang-cl and lld-link.
  • do the feature check
Thu, May 27, 8:06 PM · Restricted Project
rnk requested review of D103298: [gn] Make ubsan errors fatal, as in cmake.
Thu, May 27, 8:02 PM · Restricted Project
rnk committed rG99f023656b78: [PDB] Fix ubsan complaint about memcpy from null pointer (authored by rnk).
[PDB] Fix ubsan complaint about memcpy from null pointer
Thu, May 27, 7:49 PM
rnk accepted D103293: [clang-cl] Bump default -fms-compatibility-version to 19.14.

lgtm with the comment update

Thu, May 27, 7:42 PM · Restricted Project
rnk accepted D103297: [LLD][COFF] Reduce the maximum size of the GHASH table.

lgtm

Thu, May 27, 7:37 PM · Restricted Project
rnk added a comment to D103288: [SanCov] Properly set ABI parameter attributes.

Ew. I guess I was wrong, the getCalledFunction version was better. Sorry about that. :)

Thu, May 27, 3:21 PM · Restricted Project
rnk accepted D103288: [SanCov] Properly set ABI parameter attributes.

lgtm

Thu, May 27, 3:11 PM · Restricted Project
rnk added a comment to D96862: Allow passing /manifestdependency via #pragma comment(linker, ...).

I don't have plans to work on this, but it's a pretty straightforward starter project if anyone is interested.

Thu, May 27, 3:09 PM · Restricted Project, lld
rnk added inline comments to D103288: [SanCov] Properly set ABI parameter attributes.
Thu, May 27, 3:03 PM · Restricted Project
rnk updated subscribers of D92515: Bump MSVC required version to 19.14.

Would it be a good idea to bump the default MSVC version that Clang presents itself as to 19.14 at this point, to remedy this situation?

Thu, May 27, 3:01 PM · Restricted Project
rnk closed D43881: Add CMake option for using /DEBUG:GHASH.
Thu, May 27, 2:39 PM · Restricted Project
Herald added a project to D43881: Add CMake option for using /DEBUG:GHASH: Restricted Project.

We are moving to make ghash the default, so I don't think we need this. I have a follow-up to pass -gcodeview-ghash: D102888 I think we can close this as obsolete.

Thu, May 27, 2:39 PM · Restricted Project
rnk added a comment to D102888: [PDB] Enable parallel ghash type merging by default.

I suppose I should've updated D43881, but I made a new one at https://reviews.llvm.org/D103287. Oh well.

Thu, May 27, 2:38 PM · Restricted Project
rnk added a comment to D99707: Remove "Rewrite Symbols" from codegen pipeline.

In the absence of review from ARM, I think we should land this as is.

Thu, May 27, 2:37 PM · Restricted Project
rnk requested review of D103287: Pass -gcodeview-ghash when using clang-cl and lld-link.
Thu, May 27, 2:35 PM · Restricted Project
rnk committed rG109aac92128c: [PDB] Enable parallel ghash type merging by default (authored by rnk).
[PDB] Enable parallel ghash type merging by default
Thu, May 27, 2:20 PM
rnk closed D102888: [PDB] Enable parallel ghash type merging by default.
Thu, May 27, 2:19 PM · Restricted Project
rnk added a comment to D101806: [TargetLowering] Only inspect attributes in the arguments for ArgListEntry.

The test failures are from the libfuzzer tests, which suggests that the coverage instrumentation pass has the same problem that msan had. I see an instance of the ZExt attribute there, that doesn't match the call site:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp#L428

Thu, May 27, 1:42 PM · Restricted Project
rnk accepted D102970: [clang] [MinGW] Don't mark emutls variables as DSO local.

lgtm

Thu, May 27, 1:34 PM · Restricted Project

Wed, May 26

rnk accepted D102736: Fix tmp files being left on Windows builds..

I think this look good. Adrian, are your concerns addressed?

Wed, May 26, 4:21 PM · Restricted Project, Restricted Project
rnk accepted D101100: [ConstFold] Simplify a load's GEP operand through local aliases.

lgtm

Wed, May 26, 4:19 PM · Restricted Project

Tue, May 25

rnk added a comment to D102736: Fix tmp files being left on Windows builds..

Nice, this seems to be working out.

Tue, May 25, 2:33 PM · Restricted Project, Restricted Project
rnk added inline comments to D101100: [ConstFold] Simplify a load's GEP operand through local aliases.
Tue, May 25, 11:52 AM · Restricted Project

Fri, May 21

rnk accepted D102946: [MinGW] Mark a number of library functions unavailable for mingw targets.

lgtm

Fri, May 21, 2:56 PM · Restricted Project
rnk accepted D102899: [lit] Print full googletest commad line.

Classic.

Fri, May 21, 2:48 PM · Restricted Project
rnk accepted D102944: [Windows] Use TerminateProcess to exit without running destructors.

Yep, this is more or less exactly what I had in mind. :)

Fri, May 21, 2:34 PM · Restricted Project
rnk added a comment to D102684: [LLD] Allow disabling the early exit codepath as a build configuration.

If ExitProcess still unloads DLLs before exiting, I believe there are ways to call TerminateProcess to avoid this.

Fri, May 21, 9:58 AM · Restricted Project
rnk added a comment to D102684: [LLD] Allow disabling the early exit codepath as a build configuration.

I'm in favor of strengthening _exit to ExitProcess. Fewer configs is better. Users won't have to discover these flaky shutdown crashes and add this customization.

Fri, May 21, 9:56 AM · Restricted Project
rnk accepted D102153: [SelectionDAG] Fix argument copy elision with irregular types.

lgtm, thanks.

Fri, May 21, 9:54 AM · Restricted Project
rnk added inline comments to D102153: [SelectionDAG] Fix argument copy elision with irregular types.
Fri, May 21, 9:20 AM · Restricted Project
rnk added a comment to D102888: [PDB] Enable parallel ghash type merging by default.

@rnk as a next step, you would probably want to re-do D43881 so that -gcodeview-ghash is enabled by default when building LLVM?

Fri, May 21, 8:21 AM · Restricted Project
rnk added a comment to D102899: [lit] Print full googletest commad line.

I see test failures on Windows that appear to be related to CRLF handling. I think the CHECK-NEXT checks aren't working because something is doubling up the newlines in the test output. That seems unrelated, so I suggest dropping the -NEXTs as necessary to get it past the presubmit testing.

Fri, May 21, 8:17 AM · Restricted Project

Thu, May 20

rnk added inline comments to D102888: [PDB] Enable parallel ghash type merging by default.
Thu, May 20, 5:09 PM · Restricted Project
rnk updated the diff for D102888: [PDB] Enable parallel ghash type merging by default.
  • use /debug:noghash
Thu, May 20, 5:08 PM · Restricted Project
rnk updated the summary of D102888: [PDB] Enable parallel ghash type merging by default.
Thu, May 20, 4:49 PM · Restricted Project
rnk updated the summary of D102888: [PDB] Enable parallel ghash type merging by default.
Thu, May 20, 4:49 PM · Restricted Project
rnk added inline comments to D102871: Compiler crashing when compiling coroutine intrinsics without the fcoroutines-ts option..
Thu, May 20, 4:40 PM
rnk committed rGe73203a561b7: [PDB] Check the type server guid when ghashing (authored by rnk).
[PDB] Check the type server guid when ghashing
Thu, May 20, 4:36 PM
rnk added a comment to D102885: [PDB] Check the type server guid when ghashing.

LTGM.

If you're making /DEBUG:GHASH the default, can we switch back to the "classic" merging?

Thu, May 20, 4:36 PM · Restricted Project
rnk closed D102885: [PDB] Check the type server guid when ghashing.
Thu, May 20, 4:36 PM · Restricted Project