Page MenuHomePhabricator

scott.linder (Scott Linder)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 15 2018, 8:31 AM (47 w, 1 d)

Recent Activity

Mon, Dec 10

scott.linder added a comment to D54489: Implement -frecord-command-line (-frecord-gcc-switches).

There may be changes to some details in the LLVM patch; once they are finalized I will update the Clang patch.

Mon, Dec 10, 11:52 AM
scott.linder updated the diff for D54487: Implement llvm.commandline named metadata.

Use the same ELF section name as GCC

Mon, Dec 10, 11:51 AM
scott.linder added a comment to D54487: Implement llvm.commandline named metadata.

I'll ask the larger question of why did you pick ".LLVM.command.line" as the section name? While distasteful, there might be existing scripts that do things like:

readelf -p .GCC.command.line a.out

to dump out the arguments like

String dump of section '.GCC.command.line':

[     0]  a.c
[     4]  -mtune=generic
[    13]  -march=x86-64
[    21]  -O2
[    25]  -frecord-gcc-switches

Yes, the format of contents is not well-defined (as mentioned in the gcc man pages), but for somebody switching from a gcc-based toolchain to an LLVM-based toolchain, would they expect the sections to have the same names?

Your description says this would allow clang to add -frecord-gcc-switches (and not -frecord-llvm-switches) so would someone expect that clang would follow the module and create a '.GCC.command.line' section name?

Or perhaps also create a .GCC.command.line section that would contain 'see .LLVM.command.line for additional command line descriptions' so at least existing scripts might not get tripped up?

Or am I overthinking this from working in a high "upward-compatibility, don't break anything" environment for 30+ years?

Mon, Dec 10, 10:52 AM
scott.linder added reviewers for D54487: Implement llvm.commandline named metadata: probinson, JohnReagan, seaneveson, jhenderson.

I'm not certain who else might want to review this, but I'm adding some who have reviewed the touched files recently.

Mon, Dec 10, 8:22 AM
scott.linder updated the diff for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Rebase, ping

Mon, Dec 10, 7:39 AM

Fri, Dec 7

scott.linder created D55439: [MC] Do not consider .ifdef/.ifndef as a use.
Fri, Dec 7, 9:04 AM

Mon, Dec 3

scott.linder added a comment to D54953: [DWARFv5] Verify all-or-nothing constraint on DIFile source.

I posted to the commit first by accident; copying here:

Mon, Dec 3, 2:50 PM
scott.linder added a comment to rL348022: [DWARFv5] Verify all-or-nothing constraint on DIFile source.

I realize now that this is insufficient. The other DI* metadata which have a file: (e.g. DILocalVariable) can point to an arbitrary DIFile, and I don't confirm that it matches the source: of all other files in the same DICompileUnit.

Mon, Dec 3, 2:48 PM
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

Okay. So it's still the case that all symbols will be defined within the linkage unit; it's just that some things might need to get exposed outside of it.

LLVM does provide a dso_local attribute which you could use unconditionally on every symbol without actually changing visibility.

Mon, Dec 3, 1:52 PM
scott.linder added a comment to D53768: Add VerboseOutputStream to CompilerInstance.

Ping

Mon, Dec 3, 8:13 AM
scott.linder added a comment to D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Ping

Mon, Dec 3, 8:12 AM
scott.linder added a comment to D54487: Implement llvm.commandline named metadata.

Ping

Mon, Dec 3, 8:12 AM

Fri, Nov 30

scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

You still have the same linkage model for those other languages, right? Ultimately there's something like a kernel function that has to be declared specially and which becomes the only public interface of the code?

Fri, Nov 30, 11:39 AM
scott.linder added a comment to D53329: Generate DIFile with main program if source is not available.

The Verifier patch has landed as https://reviews.llvm.org/rL348022

Fri, Nov 30, 11:24 AM · debug-info
scott.linder committed rL348022: [DWARFv5] Verify all-or-nothing constraint on DIFile source.
[DWARFv5] Verify all-or-nothing constraint on DIFile source
Fri, Nov 30, 11:16 AM
scott.linder closed D54953: [DWARFv5] Verify all-or-nothing constraint on DIFile source.
Fri, Nov 30, 11:16 AM
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

I don't believe that is currently the case (the unrestricted linking of OCL code to OCL code via a dynamic linker), but we do have the notion of a static link step, followed by dynamic linking at runtime. The static link step is currently via IR, but we plan to support linking object files. Maybe I misunderstand the distinction between linkage and visibility, but it seems reasonable that a user would want to have e.g. a non-kernel function participate in static linking, but not be preemptible in the final shared object. The intention with this patch is to allow this with something like -fvisibility hidden without disrupting kernel symbols, which must appear in the dynsym for the reasons mentioned earlier in the thread.

Okay, this still doesn't need user-controlled symbol visibility. The basic question here is whether there is any need for anything other than kernel functions to have dynamic linkage. If not, then you really just need to mark everything as hidden except for kernels. You *could* do that marking in Sema, but frankly that seems like an implementation detail escaping into the AST, and it's just going to cause you unwanted pain and sub-optimality. (For example: -fvisibility=hidden doesn't actually change the visibility of external declarations for reasons that make a lot of sense for general-purpose environments but probably don't matter to you at all. If you're doing this to get better code-generation for references to external entities that are going to end up within the image, you actually need to add a visibility attribute to every single top-level declaration to get that effect. It's much easier to just do it in a pass on the module.)

Fri, Nov 30, 8:40 AM

Thu, Nov 29

scott.linder updated the diff for D54953: [DWARFv5] Verify all-or-nothing constraint on DIFile source.

The constraint is only per-CU. I've attempted to capture this in the latest patch, although I don't quite understand the interaction of all of the DI* metadata, so I may have still missed cases.

Thu, Nov 29, 1:26 PM

Wed, Nov 28

scott.linder updated the summary of D54953: [DWARFv5] Verify all-or-nothing constraint on DIFile source.
Wed, Nov 28, 2:25 PM

Tue, Nov 27

scott.linder updated the diff for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Rebase

Tue, Nov 27, 11:53 AM
scott.linder updated the diff for D54489: Implement -frecord-command-line (-frecord-gcc-switches).

Update documentation for new option

Tue, Nov 27, 11:17 AM
scott.linder updated the summary of D54487: Implement llvm.commandline named metadata.
Tue, Nov 27, 11:16 AM
scott.linder added a comment to D54487: Implement llvm.commandline named metadata.

Got it. Yes, SHT_NOTEs probably don't survive into the final image so SHT_PROGBITS is what was chosen. I wonder if the linker special cases on the section name to keep it from being part of a load section? Seems like a waste to read that stuff into memory at image activation time.

Tue, Nov 27, 11:14 AM
scott.linder updated the diff for D54487: Implement llvm.commandline named metadata.

Rebase

Tue, Nov 27, 11:11 AM
scott.linder added a comment to D54953: [DWARFv5] Verify all-or-nothing constraint on DIFile source.

Actually, after thinking about this I agree more with the approach of dropping or somehow warning/failing in the DWARF backend, rather than verifying at the IR level. Conceivably another DI backend which implements embedded source might not have the same restriction as DWARF, so restricting the IR doesn't seem right.

Tue, Nov 27, 8:31 AM
scott.linder added a comment to D53329: Generate DIFile with main program if source is not available.

See https://reviews.llvm.org/D54953 for the Verifier work.

Tue, Nov 27, 8:27 AM · debug-info
scott.linder created D54953: [DWARFv5] Verify all-or-nothing constraint on DIFile source.
Tue, Nov 27, 8:26 AM
scott.linder accepted D54358: [AMDGPU] Disable DAG combine at -O0.

LGTM; if there are other regressions we can address them as they come up, but I don't see any major ones now.

Tue, Nov 27, 7:10 AM

Fri, Nov 16

scott.linder updated the diff for D54489: Implement -frecord-command-line (-frecord-gcc-switches).

Update documentation for new option and error in the driver when generating for unsupported object-file format.

Fri, Nov 16, 12:34 PM
scott.linder updated the diff for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Use unique_ptr for HSAMetadataStreamer

Fri, Nov 16, 10:47 AM
scott.linder added a comment to D53329: Generate DIFile with main program if source is not available.

Thanks!

So I can see where/how this fails now - the LLVM backend seems to require that if any file has embedded source, they all do. Would you be able to/would you mind adding a debug info verifier check for this? That'd help make this fail sooner. (@scott.linder - perhaps you'd be able to/interested in doing this - looks like you wrote the initial implementation?)

Beyond that, then the question is whether this fix is the right way to satisfy that requirement (I'm assuming the requirement is reasonable - though I've not looked at the embedded source support & have no idea if there's a way/it's reasonable to support some embedded source and some not). My big question & I've tried to do some debugging to better understand this - but don't have the time to dive much further into it (& haven't really figured it out as yet): Why is this source not accessible through this API at this point? is the fallback to the main file always accurate?

(as far as I got was figuring out that SourceManager::getBufferData had an SLoc with isFile == true for the header, but isFile was false for the SLocs related to the .cpp file. Can't say I know why that is/what that means & would like to understand that (or someone else more familiar with this stuff who already knows can review this code) before approving this patch - if you could help explain this, that'd be great)

Fri, Nov 16, 10:34 AM · debug-info

Thu, Nov 15

scott.linder added a comment to D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

what is the rationale for putting the metadata verifier in binary format instead of support?

Thu, Nov 15, 12:51 PM
scott.linder committed rL346992: [AMDGPU] Update code object metadata format documentation.
[AMDGPU] Update code object metadata format documentation
Thu, Nov 15, 12:49 PM
scott.linder closed D53445: [AMDGPU] Update code object metadata format documentation.
Thu, Nov 15, 12:49 PM
scott.linder added inline comments to D54358: [AMDGPU] Disable DAG combine at -O0.
Thu, Nov 15, 12:36 PM
scott.linder updated the diff for D53445: [AMDGPU] Update code object metadata format documentation.

Preserve docs for code object v2 metadata

Thu, Nov 15, 11:50 AM
scott.linder closed D51223: Update tests for new YAMLIO polymorphic traits.

r346884

Thu, Nov 15, 10:53 AM
scott.linder accepted D51223: Update tests for new YAMLIO polymorphic traits.

r346884

Thu, Nov 15, 10:53 AM
scott.linder committed rL346978: [BinaryFormat] Add MsgPackTypes.
[BinaryFormat] Add MsgPackTypes
Thu, Nov 15, 10:52 AM
scott.linder closed D48175: [BinaryFormat] Add MsgPackTypes.
Thu, Nov 15, 10:52 AM
scott.linder added a comment to D54489: Implement -frecord-command-line (-frecord-gcc-switches).

Looking a bit further, it seems some other object-file formats have conventions for naming (e.g. Mach-O sections are _snake_case) and so .LLVM.command.line may not end up being what we want universally.

Thu, Nov 15, 8:51 AM
scott.linder added inline comments to D54489: Implement -frecord-command-line (-frecord-gcc-switches).
Thu, Nov 15, 6:36 AM

Wed, Nov 14

scott.linder updated the diff for D54489: Implement -frecord-command-line (-frecord-gcc-switches).

Change canonical option name to -frecord-command-line and add an alias from -frecord-gcc-switches

Wed, Nov 14, 2:48 PM
scott.linder updated the diff for D54487: Implement llvm.commandline named metadata.

Remove new ".commandline" ASM directive in favor of standard ones (e.g. .section, .ascii)

Wed, Nov 14, 2:11 PM
scott.linder added a comment to D53768: Add VerboseOutputStream to CompilerInstance.

Ping

Wed, Nov 14, 12:15 PM
scott.linder committed rL346884: [Support] Teach YAMLIO about polymorphic types.
[Support] Teach YAMLIO about polymorphic types
Wed, Nov 14, 11:43 AM
scott.linder committed rCTE346884: [Support] Teach YAMLIO about polymorphic types.
[Support] Teach YAMLIO about polymorphic types
Wed, Nov 14, 11:43 AM
scott.linder committed rC346884: [Support] Teach YAMLIO about polymorphic types.
[Support] Teach YAMLIO about polymorphic types
Wed, Nov 14, 11:43 AM
scott.linder closed D48144: [Support] Teach YAMLIO about polymorphic types.
Wed, Nov 14, 11:42 AM
scott.linder edited parent revisions for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object, added: 1; removed: 1.
Wed, Nov 14, 9:32 AM
scott.linder added a child revision for D53445: [AMDGPU] Update code object metadata format documentation: D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.
Wed, Nov 14, 9:32 AM
scott.linder removed a child revision for D47549: [AMDGPU] Update code object metadata format documentation: D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.
Wed, Nov 14, 9:32 AM
scott.linder added a comment to D54358: [AMDGPU] Disable DAG combine at -O0.

Sorry for taking so long to look at this. I am running some OCL conformance tests to get an idea of what impact this will have in terms of regressions, but if we believe this is the correct thing to do I don't think that should have any impact on when it is merged anyway.

Wed, Nov 14, 8:54 AM
scott.linder updated subscribers of D54489: Implement -frecord-command-line (-frecord-gcc-switches).
Wed, Nov 14, 7:46 AM
scott.linder updated subscribers of D54487: Implement llvm.commandline named metadata.
Wed, Nov 14, 7:46 AM
scott.linder added a comment to D54487: Implement llvm.commandline named metadata.

I don't really see a need for a new directive, it's convenient but does nothing you couldn't do with existing directives.
It does have some downside, in that it's supported only by the LLVM integrated assembler.

Wed, Nov 14, 7:46 AM

Tue, Nov 13

scott.linder added a comment to D54489: Implement -frecord-command-line (-frecord-gcc-switches).

I realize that you're probably striving for option compatibility with gcc, but continuing to name it -frecord-gcc-switches when it actually records Clang switches seems weird to me. It almost sounds like something that would dump gcc equivalents of all Clang options, or maybe let you know which Clang options you've used that match gcc options. Either way, by the name -- if you aren't familiar with the gcc option -- it doesn't read like it records Clang options.

Would it be that bad to name it -frecord-clang-switches? Or just -frecord-switches?

Tue, Nov 13, 12:45 PM
scott.linder added a comment to D54487: Implement llvm.commandline named metadata.

I know you are just trying to match gcc behavior, but do you know why they use SHT_PROGBITS instead of something like SHT_NOTE for the information? On OpenVMS, we use .note entries to record compilation date/time, compiler version, command line, etc. information that is later used by our linker to print out detail link maps. Or is the intention that the information get propagated all the way to the final executable or shared library?

Tue, Nov 13, 12:42 PM
scott.linder added a reviewer for D54489: Implement -frecord-command-line (-frecord-gcc-switches): rjmccall.
Tue, Nov 13, 12:29 PM
scott.linder added a parent revision for D54489: Implement -frecord-command-line (-frecord-gcc-switches): D54487: Implement llvm.commandline named metadata.
Tue, Nov 13, 12:10 PM
scott.linder added a child revision for D54487: Implement llvm.commandline named metadata: D54489: Implement -frecord-command-line (-frecord-gcc-switches).
Tue, Nov 13, 12:10 PM
scott.linder created D54489: Implement -frecord-command-line (-frecord-gcc-switches).
Tue, Nov 13, 12:08 PM
scott.linder created D54487: Implement llvm.commandline named metadata.
Tue, Nov 13, 12:06 PM
scott.linder updated the diff for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Update getAMDGPUNote param name to match its new type

Tue, Nov 13, 10:47 AM
scott.linder updated the diff for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Add missing AMDGPUNote type

Tue, Nov 13, 9:09 AM
scott.linder added a comment to D48144: [Support] Teach YAMLIO about polymorphic types.

The heavy use of templates does lead to some awkward usage, but it can also make some common use-cases very natural. I agree there is probably room to improve, but it is orthogonal to this patch.

Tue, Nov 13, 7:47 AM
scott.linder updated the diff for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Rebase tools/llvm-readobj/ELFDumper.cpp

Tue, Nov 13, 7:29 AM

Mon, Nov 12

scott.linder added a reviewer for D54301: [AMDGPU] Derive GCNSubtarget from MF to get overridden target features: kzhuravl.

I don't quite understand what distinguishes the two versions of MCSubtargetInfo (getSTI() vs STM), but it appears a patch from Konstantin earlier this year changed this specific instance from STM.getFeatureBits() to getSTI(). Changing this back LGTM but I don't know what the rationale was in the first place, if it wasn't a typo, so I don't want to sign off without asking.

Mon, Nov 12, 12:59 PM

Nov 6 2018

scott.linder added a comment to D52894: Clarify definition of "indirect call" in CallSite.

Please rebase on top of https://reviews.llvm.org/D52537 .

Nov 6 2018, 1:59 PM
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

I don't believe that is currently the case (the unrestricted linking of OCL code to OCL code via a dynamic linker), but we do have the notion of a static link step, followed by dynamic linking at runtime. The static link step is currently via IR, but we plan to support linking object files. Maybe I misunderstand the distinction between linkage and visibility, but it seems reasonable that a user would want to have e.g. a non-kernel function participate in static linking, but not be preemptible in the final shared object. The intention with this patch is to allow this with something like -fvisibility hidden without disrupting kernel symbols, which must appear in the dynsym for the reasons mentioned earlier in the thread.

Nov 6 2018, 1:46 PM
scott.linder updated the diff for D52894: Clarify definition of "indirect call" in CallSite.

I still think disambiguating "indirect call" in LLVM is worthwhile, so here is another attempt at addressing feedback.

Nov 6 2018, 12:20 PM

Nov 5 2018

scott.linder retitled D53153: [OpenCL] Mark kernel functions with default visibility from [OpenCL] Mark namespace scope variables and kernel functions with default visibility to [OpenCL] Mark kernel functions with default visibility.
Nov 5 2018, 1:25 PM
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

Ping

Nov 5 2018, 1:09 PM
scott.linder added a comment to D53768: Add VerboseOutputStream to CompilerInstance.

Ping

Nov 5 2018, 1:09 PM
scott.linder added a comment to D53445: [AMDGPU] Update code object metadata format documentation.

Ping

Nov 5 2018, 12:55 PM

Oct 31 2018

scott.linder committed rL345770: [SelectionDAG] Handle constant range [0,1) in lowerRangeToAssertZExt.
[SelectionDAG] Handle constant range [0,1) in lowerRangeToAssertZExt
Oct 31 2018, 1:00 PM
scott.linder closed D53888: [SelectionDAG] Handle constant range [0,1) in lowerRangeToAssertZExt.
Oct 31 2018, 1:00 PM
scott.linder committed rL345763: [AMDGPU] Remove FeatureVGPRSpilling.
[AMDGPU] Remove FeatureVGPRSpilling
Oct 31 2018, 11:56 AM
scott.linder closed D53829: [AMDGPU] Remove FeatureVGPRSpilling.
Oct 31 2018, 11:56 AM
scott.linder updated the diff for D53829: [AMDGPU] Remove FeatureVGPRSpilling.

Remove unused AMDGPUSubtarget::EnableVGPRSpilling; I missed this in the first patch.

Oct 31 2018, 11:44 AM
scott.linder updated the diff for D51223: Update tests for new YAMLIO polymorphic traits.

@klimek is there anyone I should add to take a look at this? As far as the YAML is concerned I believe this is a non-functional change.

Oct 31 2018, 8:43 AM
scott.linder updated the diff for D53888: [SelectionDAG] Handle constant range [0,1) in lowerRangeToAssertZExt.

Add lit LABEL for test case

Oct 31 2018, 7:56 AM
scott.linder updated the diff for D48144: [Support] Teach YAMLIO about polymorphic types.

Address feedback

Oct 31 2018, 7:43 AM

Oct 30 2018

scott.linder created D53888: [SelectionDAG] Handle constant range [0,1) in lowerRangeToAssertZExt.
Oct 30 2018, 1:31 PM

Oct 29 2018

scott.linder created D53829: [AMDGPU] Remove FeatureVGPRSpilling.
Oct 29 2018, 12:45 PM
scott.linder added a comment to D48144: [Support] Teach YAMLIO about polymorphic types.

Ping

Oct 29 2018, 7:04 AM
scott.linder added a comment to D53445: [AMDGPU] Update code object metadata format documentation.

Ping

Oct 29 2018, 7:03 AM

Oct 26 2018

scott.linder created D53768: Add VerboseOutputStream to CompilerInstance.
Oct 26 2018, 9:40 AM
scott.linder committed rL345382: [AMDGPU] Add a pass to promote bitcast calls.
[AMDGPU] Add a pass to promote bitcast calls
Oct 26 2018, 6:21 AM
scott.linder closed D52741: [AMDGPU] Add an AMDGPU pass to promote bitcast calls.
Oct 26 2018, 6:21 AM

Oct 25 2018

scott.linder updated the diff for D52741: [AMDGPU] Add an AMDGPU pass to promote bitcast calls.

Move addPass for new pass

Oct 25 2018, 1:04 PM

Oct 23 2018

scott.linder updated the diff for D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.

Rebase and update:

Oct 23 2018, 1:43 PM
scott.linder updated the diff for D52741: [AMDGPU] Add an AMDGPU pass to promote bitcast calls.

Add test for promoting invoke instruction

Oct 23 2018, 9:12 AM
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

Beyond constructors/destructors I believe an API which we implement through access to dynamic symbols for global variable is clGetProgramBuildInfo(..., CL_PROGRAM_BUILD_GLOBAL_VARIABLE_TOTAL_SIZE, ...) which is defined as "The total amount of storage, in bytes, used by program variables in the global address space."

Presumably this is looking at the size of a certain section / segment of the image. It's unclear to me why symbol visibility would affect this in any way.

Oct 23 2018, 7:44 AM
scott.linder updated the diff for D53153: [OpenCL] Mark kernel functions with default visibility.

Don't mark namespace-scope global variable declarations in OpenCL with explicit default visibility

Oct 23 2018, 7:25 AM
scott.linder updated the diff for D53445: [AMDGPU] Update code object metadata format documentation.

Update note type value

Oct 23 2018, 7:08 AM
scott.linder added a comment to D53445: [AMDGPU] Update code object metadata format documentation.

In v2 asm, we had the following:

.option.machine_version_major
.option.machine_version_minor
.option.machine_version_stepping

What are the equivalents for v3 asm? The updates you made do not seem to describe those... Unless I missed it.

Oct 23 2018, 7:07 AM

Oct 19 2018

scott.linder updated the diff for D53445: [AMDGPU] Update code object metadata format documentation.

Update note type value.

Oct 19 2018, 2:26 PM
scott.linder added a comment to D52741: [AMDGPU] Add an AMDGPU pass to promote bitcast calls.

Ping

Oct 19 2018, 12:12 PM
scott.linder created D53445: [AMDGPU] Update code object metadata format documentation.
Oct 19 2018, 11:56 AM