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 (57 w, 4 d)

Recent Activity

Yesterday

scott.linder accepted D58512: AMDGPU/GlobalISel: Insert waterfall loop for vector indexing.

LGTM then, but like I said I'm not very familiar with GlobalISel so more eyes might be good

Fri, Feb 22, 9:31 AM
scott.linder added inline comments to D58512: AMDGPU/GlobalISel: Insert waterfall loop for vector indexing.
Fri, Feb 22, 8:49 AM
scott.linder added a comment to D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis.

Does anyone have an opinion on returning negative branch targets (e.g. <keep_symbol+0xfffffffffffe0018>)? I don't know how this would ever come up in hardware anyway, or what the hardware would do, but it doesn't seem very helpful in the disassembly.

The hardware doesn't know about the symbol? Do you mean for overflow?

Fri, Feb 22, 7:55 AM · Restricted Project

Thu, Feb 21

scott.linder added a comment to D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis.

Does anyone have an opinion on returning negative branch targets (e.g. <keep_symbol+0xfffffffffffe0018>)? I don't know how this would ever come up in hardware anyway, or what the hardware would do, but it doesn't seem very helpful in the disassembly.

Thu, Feb 21, 9:19 AM · Restricted Project

Wed, Feb 20

scott.linder updated the diff for D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis.

Improved error message.

Wed, Feb 20, 2:25 PM · Restricted Project
scott.linder added inline comments to D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis.
Wed, Feb 20, 2:24 PM · Restricted Project
scott.linder updated the diff for D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis.

Address feedback. Sign-extend all s_branch immediates and change the assembler syntax to represent these as true negative numbers.

Wed, Feb 20, 1:54 PM · Restricted Project
scott.linder added inline comments to D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis.
Wed, Feb 20, 8:01 AM · Restricted Project

Tue, Feb 19

scott.linder created D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis.
Tue, Feb 19, 11:39 AM · Restricted Project

Tue, Feb 12

scott.linder committed rG80a1ee46d87b: [AMDGPU] Require at least protected visibility for certain symbols (authored by scott.linder).
[AMDGPU] Require at least protected visibility for certain symbols
Tue, Feb 12, 10:31 AM
scott.linder committed rC353870: [AMDGPU] Require at least protected visibility for certain symbols.
[AMDGPU] Require at least protected visibility for certain symbols
Tue, Feb 12, 10:31 AM
scott.linder committed rL353870: [AMDGPU] Require at least protected visibility for certain symbols.
[AMDGPU] Require at least protected visibility for certain symbols
Tue, Feb 12, 10:31 AM
scott.linder closed D56871: [AMDGPU] Require at least protected visibility for certain symbols.
Tue, Feb 12, 10:31 AM · Restricted Project

Mon, Feb 11

scott.linder committed rG72a0f4e8db40: [IRReader] Expose getLazyIRModule (authored by scott.linder).
[IRReader] Expose getLazyIRModule
Mon, Feb 11, 2:01 PM
scott.linder committed rL353755: [IRReader] Expose getLazyIRModule.
[IRReader] Expose getLazyIRModule
Mon, Feb 11, 2:01 PM
scott.linder closed D56203: [IRReader] Expose getLazyIRModule.
Mon, Feb 11, 2:01 PM · Restricted Project
scott.linder added a comment to D56203: [IRReader] Expose getLazyIRModule.

Ping

Mon, Feb 11, 9:50 AM · Restricted Project
scott.linder added a comment to D56871: [AMDGPU] Require at least protected visibility for certain symbols.

Ping

Mon, Feb 11, 9:48 AM · Restricted Project

Fri, Feb 8

Herald added a project to D57028: [AMDGPU] Added MsgPack format PAL metadata: Restricted Project.
Fri, Feb 8, 8:16 AM · Restricted Project

Tue, Feb 5

scott.linder committed rGe2c584741469: [AMDGPU] Consider XOR in waterfall loop as a terminator (authored by scott.linder).
[AMDGPU] Consider XOR in waterfall loop as a terminator
Tue, Feb 5, 11:51 AM
scott.linder committed rL353207: [AMDGPU] Consider XOR in waterfall loop as a terminator.
[AMDGPU] Consider XOR in waterfall loop as a terminator
Tue, Feb 5, 11:50 AM
scott.linder closed D57703: [AMDGPU] Consider XOR in waterfall loop as a terminator.
Tue, Feb 5, 11:50 AM · Restricted Project
scott.linder updated the diff for D57703: [AMDGPU] Consider XOR in waterfall loop as a terminator.

I think MIR is the best place to look; this now tests that the right pseudo is produced and that spills don't interleave the terminators.

Tue, Feb 5, 7:35 AM · Restricted Project

Mon, Feb 4

scott.linder updated the diff for D57703: [AMDGPU] Consider XOR in waterfall loop as a terminator.

I agree the test does not test much at all, it was just the minimum I could think of considering none of our existing tests notice the change. I'm not sure what pass you mean by 'isel', but 'stop-after=amdgpu-isel' is too early to see the SI_INDIRECT_SRC_* psuedo expanded.

Mon, Feb 4, 12:47 PM · Restricted Project
scott.linder added a comment to D57027: [AMDGPU] Factored PAL metadata handling out into its own class.
In D57027#1383432, @tpr wrote:

The code looks good to me, but I'm wondering why the move from doing this in AsmPrinter::EmitEndOfAsmFile to TargetStreamer::finish? It seems like it is fine to emit directives in either, but I don't understand why the change here.

The idea was so that a disassembler disassembling into a TargetStreamer would go through the same code to emit the directives.

Mon, Feb 4, 12:09 PM · Restricted Project
scott.linder committed rGd19d1972217b: [AMDGPU] Support emitting GOT relocations for function calls (authored by scott.linder).
[AMDGPU] Support emitting GOT relocations for function calls
Mon, Feb 4, 12:00 PM
scott.linder committed rL353083: [AMDGPU] Support emitting GOT relocations for function calls.
[AMDGPU] Support emitting GOT relocations for function calls
Mon, Feb 4, 12:00 PM
scott.linder closed D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Mon, Feb 4, 11:59 AM · Restricted Project
scott.linder accepted D57024: [AMDGPU] Switched HSA metadata to use MsgPackDocument.

LGTM

Mon, Feb 4, 11:55 AM · Restricted Project
scott.linder created D57703: [AMDGPU] Consider XOR in waterfall loop as a terminator.
Mon, Feb 4, 11:50 AM · Restricted Project
scott.linder removed a parent revision for D57416: [AMDGPU] Support emitting GOT relocations for function calls: D57605: [AMDGPU] Mark test functions with hidden visibility.
Mon, Feb 4, 8:43 AM · Restricted Project
scott.linder removed a child revision for D57605: [AMDGPU] Mark test functions with hidden visibility: D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Mon, Feb 4, 8:43 AM · Restricted Project

Fri, Feb 1

scott.linder abandoned D53153: [OpenCL] Mark kernel functions with default visibility.

See https://reviews.llvm.org/D56871

Fri, Feb 1, 2:02 PM
scott.linder added a child revision for D57605: [AMDGPU] Mark test functions with hidden visibility: D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Fri, Feb 1, 2:00 PM · Restricted Project
scott.linder added a parent revision for D57416: [AMDGPU] Support emitting GOT relocations for function calls: D57605: [AMDGPU] Mark test functions with hidden visibility.
Fri, Feb 1, 2:00 PM · Restricted Project
scott.linder removed a child revision for D53153: [OpenCL] Mark kernel functions with default visibility: D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Fri, Feb 1, 1:58 PM
scott.linder removed a parent revision for D57416: [AMDGPU] Support emitting GOT relocations for function calls: D53153: [OpenCL] Mark kernel functions with default visibility.
Fri, Feb 1, 1:58 PM · Restricted Project
scott.linder added inline comments to D57027: [AMDGPU] Factored PAL metadata handling out into its own class.
Fri, Feb 1, 1:55 PM · Restricted Project
Herald added a project to D57027: [AMDGPU] Factored PAL metadata handling out into its own class: Restricted Project.

The code looks good to me, but I'm wondering why the move from doing this in AsmPrinter::EmitEndOfAsmFile to TargetStreamer::finish? It seems like it is fine to emit directives in either, but I don't understand why the change here.

Fri, Feb 1, 1:48 PM · Restricted Project
scott.linder committed rL352920: [AMDGPU] Mark test functions with hidden visibility.
[AMDGPU] Mark test functions with hidden visibility
Fri, Feb 1, 1:23 PM
scott.linder closed D57605: [AMDGPU] Mark test functions with hidden visibility.
Fri, Feb 1, 1:23 PM · Restricted Project
scott.linder updated the diff for D57416: [AMDGPU] Support emitting GOT relocations for function calls.

Add test for tail call through GOT

Fri, Feb 1, 12:20 PM · Restricted Project
scott.linder accepted D57025: [MsgPack] Removed MsgPackTypes.

LGTM, pending parents of course.

Fri, Feb 1, 11:51 AM · Restricted Project
Herald added a project to D57024: [AMDGPU] Switched HSA metadata to use MsgPackDocument: Restricted Project.

Thank you for updating this! I only have one small suggestion, but otherwise it LGTM.

Fri, Feb 1, 11:50 AM · Restricted Project
scott.linder accepted D57023: [MsgPack] New MsgPackDocument class.

LGTM, sorry for the delay in reviewing this.

Fri, Feb 1, 11:42 AM · Restricted Project
scott.linder added a parent revision for D57416: [AMDGPU] Support emitting GOT relocations for function calls: D53153: [OpenCL] Mark kernel functions with default visibility.
Fri, Feb 1, 11:10 AM · Restricted Project
scott.linder added a child revision for D53153: [OpenCL] Mark kernel functions with default visibility: D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Fri, Feb 1, 11:10 AM
scott.linder added inline comments to D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Fri, Feb 1, 11:09 AM · Restricted Project
scott.linder updated the diff for D57416: [AMDGPU] Support emitting GOT relocations for function calls.

Address feedback

Fri, Feb 1, 11:08 AM · Restricted Project
scott.linder created D57605: [AMDGPU] Mark test functions with hidden visibility.
Fri, Feb 1, 11:08 AM · Restricted Project

Thu, Jan 31

scott.linder updated the diff for D57416: [AMDGPU] Support emitting GOT relocations for function calls.

I've tried to implement carrying a copy of the function global through the DAG from the beginning, rather than recovering it after legalizing the GlobalAddress. I don't know if this is a reasonable approach, but I figured I would post something and see what you think.

Thu, Jan 31, 1:54 PM · Restricted Project

Wed, Jan 30

scott.linder added inline comments to D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Wed, Jan 30, 1:49 PM · Restricted Project

Tue, Jan 29

scott.linder added inline comments to D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Tue, Jan 29, 3:09 PM · Restricted Project
scott.linder created D57416: [AMDGPU] Support emitting GOT relocations for function calls.
Tue, Jan 29, 2:14 PM · Restricted Project

Mon, Jan 28

scott.linder added a comment to D56871: [AMDGPU] Require at least protected visibility for certain symbols.

Ping

Mon, Jan 28, 2:08 PM · Restricted Project
scott.linder committed rL352414: [MC] Do not consider .ifdef/.ifndef as a use.
[MC] Do not consider .ifdef/.ifndef as a use
Mon, Jan 28, 11:33 AM
scott.linder closed D55439: [MC] Do not consider .ifdef/.ifndef as a use.
Mon, Jan 28, 11:33 AM
scott.linder committed rC352391: Add -fapply-global-visibility-to-externs for -cc1.
Add -fapply-global-visibility-to-externs for -cc1
Mon, Jan 28, 9:12 AM
scott.linder committed rL352391: Add -fapply-global-visibility-to-externs for -cc1.
Add -fapply-global-visibility-to-externs for -cc1
Mon, Jan 28, 9:12 AM
scott.linder closed D56868: Add -fapply-global-visibility-to-externs for -cc1.
Mon, Jan 28, 9:12 AM
scott.linder added a comment to D55439: [MC] Do not consider .ifdef/.ifndef as a use.

Ping

Mon, Jan 28, 8:33 AM

Jan 18 2019

scott.linder updated the diff for D56871: [AMDGPU] Require at least protected visibility for certain symbols.

Update option name

Jan 18 2019, 10:47 AM · Restricted Project
scott.linder updated the diff for D56868: Add -fapply-global-visibility-to-externs for -cc1.

No worries, I agree that we don't gain much with a shorter flag here; explicit seems preferable.

Jan 18 2019, 10:46 AM
scott.linder updated the diff for D56871: [AMDGPU] Require at least protected visibility for certain symbols.

Update option name

Jan 18 2019, 8:51 AM · Restricted Project
scott.linder updated the diff for D56868: Add -fapply-global-visibility-to-externs for -cc1.

Update the -cc1 option name and docs; also update the LangOpt and docs.

Jan 18 2019, 8:47 AM

Jan 17 2019

scott.linder updated the diff for D56871: [AMDGPU] Require at least protected visibility for certain symbols.

Add missing flag to tests

Jan 17 2019, 1:55 PM · Restricted Project
scott.linder updated the diff for D56868: Add -fapply-global-visibility-to-externs for -cc1.

Remove driver options

Jan 17 2019, 1:47 PM
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

That sounds reasonable to me. I had already posted a patch with the Driver options, but I will update it to only include the -cc1 version.

Jan 17 2019, 1:30 PM
scott.linder abandoned D52891: [AMDGPU] Add -fvisibility-amdgpu-non-kernel-functions.

Will be superseded by either https://reviews.llvm.org/D53153 or https://reviews.llvm.org/D56871

Jan 17 2019, 11:45 AM
scott.linder updated the diff for D56203: [IRReader] Expose getLazyIRModule.

Rebase and ping

Jan 17 2019, 11:44 AM · Restricted Project
scott.linder added a parent revision for D56871: [AMDGPU] Require at least protected visibility for certain symbols: D56868: Add -fapply-global-visibility-to-externs for -cc1.
Jan 17 2019, 11:37 AM · Restricted Project
scott.linder added a child revision for D56868: Add -fapply-global-visibility-to-externs for -cc1: D56871: [AMDGPU] Require at least protected visibility for certain symbols.
Jan 17 2019, 11:37 AM
scott.linder created D56871: [AMDGPU] Require at least protected visibility for certain symbols.
Jan 17 2019, 11:36 AM · Restricted Project
scott.linder changed the repository for D56868: Add -fapply-global-visibility-to-externs for -cc1 from rL LLVM to rC Clang.
Jan 17 2019, 11:35 AM
scott.linder created D56868: Add -fapply-global-visibility-to-externs for -cc1.
Jan 17 2019, 11:33 AM
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

I think we need the option to be in the driver, not just -cc1. We are moving towards the clang driver being the authority on how we do offline compilation for AMDGPU, and hiding this in cc1 doesn't help us.

Jan 17 2019, 9:55 AM
scott.linder updated the diff for D55439: [MC] Do not consider .ifdef/.ifndef as a use.

Rebase and extend tests

Jan 17 2019, 8:57 AM
scott.linder added a comment to D55439: [MC] Do not consider .ifdef/.ifndef as a use.

Ping

Jan 17 2019, 7:57 AM

Jan 15 2019

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

It sounds like -fextern-visiblity= isn't something we really want, then? I think it would have to be implemented in the abstract linkage computer.

Jan 15 2019, 2:23 PM

Jan 14 2019

scott.linder added a comment to D53329: Generate DIFile with main program if source is not available.

@dblaikie @scott.linder Have you got time to look at this issue? Looks like a trivial test case will be able to reproduce the problem. I have the simple example in the previous comments. Practically, -gdwarf-5 -gembed-source is broken now.

Jan 14 2019, 1:38 PM · debug-info
scott.linder added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

I think -fvisibility=hidden isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works. But after this discussion, I'm prepared to accept that (1) we should have some sort of single-image compiler mode that implies that all symbols are defined within the image and (2) you can make your toolchain imply that together with -fvisibility=hidden and then have specific symbols opt in to non-hidden visibility if they need to be accessible to whatever loader / runtime you have.

It seems that explicit visibility attributes on external symbol references (e.g. __attribute__((visibility("hidden"))) extern int foo;) are respected in Clang, so I don't understand the rationale for global visibility controls not applying to them as well. Can you describe why this is the case?

Well, one answer is that that's the behavior that GCC defined when they added global visibility controls, and it would be unreasonable for Clang to deviate in such a fundamental way. However, we do deviate in some other ways in our interpretation of visibility attributes, so that's not a totally satisfactory answer.

The stronger answer is that GCC has a very good reason for this behavior. Global visibility controls apply to all code in the translation unit. While programmers often mentally distinguish between "project" headers (which they control) and "library" headers (which they do not), that is not a difference that is known to the compiler. Therefore, if global visibility controls applied to external symbols, the compiler would end up assuming that all external symbols not annotated as default are provided by the current image. That assumption would have been incompatible with essentially all existing headers for dynamic libraries on GCC's core targets, including system headers; in other words, it would have caused ubiquitous miscompiles.* It would be completely unreasonable to expect all those headers to be changed just for the benefit of a single new compiler feature. Therefore, global visibility controls do not apply to external symbols because doing so would make it impossible to actually enable global visibility controls.

  • GCC's rule helps a great deal even in C++, which is generally more sensitive to global visibility: you can typically use an unannotated C++ header under -fvisibility=hidden and have no problems unless you start using its types as exceptions (and GCC tries to work around that, too) or comparing the addresses of inline functions or template instantiations.

    John.
Jan 14 2019, 11:38 AM

Jan 10 2019

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

I think -fvisibility=hidden isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works. But after this discussion, I'm prepared to accept that (1) we should have some sort of single-image compiler mode that implies that all symbols are defined within the image and (2) you can make your toolchain imply that together with -fvisibility=hidden and then have specific symbols opt in to non-hidden visibility if they need to be accessible to whatever loader / runtime you have.

Jan 10 2019, 12:35 PM

Jan 2 2019

scott.linder created D56203: [IRReader] Expose getLazyIRModule.
Jan 2 2019, 9:14 AM · Restricted Project

Dec 28 2018

scott.linder updated the diff for D55439: [MC] Do not consider .ifdef/.ifndef as a use.

Rebase and ping

Dec 28 2018, 9:41 AM

Dec 14 2018

scott.linder closed D54489: Implement -frecord-command-line (-frecord-gcc-switches).

rL349155

Dec 14 2018, 7:43 AM
scott.linder committed rC349155: Implement -frecord-command-line (-frecord-gcc-switches).
Implement -frecord-command-line (-frecord-gcc-switches)
Dec 14 2018, 7:42 AM
scott.linder committed rL349155: Implement -frecord-command-line (-frecord-gcc-switches).
Implement -frecord-command-line (-frecord-gcc-switches)
Dec 14 2018, 7:41 AM
scott.linder closed D54487: Implement llvm.commandline named metadata.
Dec 14 2018, 7:41 AM

Dec 13 2018

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

So we're using the same command line option as GCC to produce something in the same section as GCC but formatting that section incompatibly with GCC? That combination of choices does not seem like a good idea.

Dec 13 2018, 1:27 PM
scott.linder updated the diff for D54489: Implement -frecord-command-line (-frecord-gcc-switches).

Update documentation to match section naming change, and to explicitly note that the section format differs from the equivalent GCC format.

Dec 13 2018, 9:17 AM

Dec 12 2018

scott.linder updated the diff for D54487: Implement llvm.commandline named metadata.

Address feedback. The verifier appears to bail on the first failure, at least with llc/llvm-as.

Dec 12 2018, 12:45 PM
scott.linder committed rL348963: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.
[AMDGPU] Emit MessagePack HSA Metadata for v3 code object
Dec 12 2018, 11:43 AM
scott.linder closed D48179: [AMDGPU] Emit MessagePack HSA Metadata for v3 code object.
Dec 12 2018, 11:42 AM

Dec 10 2018

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.

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

Use the same ELF section name as GCC

Dec 10 2018, 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?

Dec 10 2018, 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.

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

Rebase, ping

Dec 10 2018, 7:39 AM

Dec 7 2018

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

Dec 3 2018

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:

Dec 3 2018, 2:50 PM