Page MenuHomePhabricator

greened (David Greene)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 1 2015, 10:19 AM (342 w, 1 d)

Recent Activity

Yesterday

greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

D117694 is the revision adding the filtering capability. We could improve this register scrubbing for more filtered cases but I'd prefer to do that as a later enhancement.

Wed, Jan 19, 9:41 AM · Restricted Project
greened requested review of D117694: [UpdateTestChecks] Add --filter and --filter-out options.
Wed, Jan 19, 9:40 AM · Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

A patch that only exchanges the order of the first add to

; CHECK-NEXT:    add %R0, %R2
; CHECK-NEXT:    add %R0, %R1

Then, the other use and def of %R1 and %R2 will remain unchanged.
But if you are numbering the register by order, all the other %R1 and %R2 will be changed while the expected change won't be shown. This is rather confusing.

Wed, Jan 19, 7:11 AM · Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Sorry if this has been asked before, but i think the main question here is: what's the target audience here?

Wed, Jan 19, 7:04 AM · Restricted Project

Tue, Jan 18

greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Although the new regexp looks much concise, you still haven't solved the second question @hvdijk raised.

Tue, Jan 18, 1:39 PM · Restricted Project
greened updated the diff for D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Fixed non-clobber register def regexp for x86. It was using lookbehind instead of negative lookahead for vector mnemonics.

Tue, Jan 18, 8:49 AM · Restricted Project

Fri, Jan 14

greened updated the diff for D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Clarified comments.

Fri, Jan 14, 8:28 AM · Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

It might be better if you do this for a target other than x86 - if generating arm/aarch64 codegen tests with the update scripts are a concern for those teams, using it there makes more sense, plus they have simpler register sets.

Fri, Jan 14, 8:22 AM · Restricted Project
greened updated the diff for D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Made the FileCheck variable names more generic, to limit diffs between test updates. Names are now based on register class with a uniquifying count that tracks redefinitions of registers so that changes in register def-use changes can be seen.

Fri, Jan 14, 8:21 AM · Restricted Project

Thu, Jan 13

greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

I think I can also shorten up the regexps by being smarter about what kind of register is used in the asm. I'll work on that as well. It should make things much more readable and will flag changes in the register class, which is probably desirable.

Thu, Jan 13, 8:05 AM · Restricted Project

Wed, Jan 12

greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

All of the examples you have provided show non-register-changes in the same tests, so the tests would still need re-generating. After re-generating, would all the register name updates still not be part of the diff on exactly the same lines?

Wed, Jan 12, 2:31 PM · Restricted Project
greened added inline comments to D98230: [LSR] Add reconciliation of unfoldable offsets.
Wed, Jan 12, 1:53 PM · Restricted Project
greened added a comment to D115433: [CommandLine] Reset option to its default if its Default field is undefined.

IIUC, the point of this change is not to alter the behavior for options that have a cl::init, but rather to alter behavior for options that _don't_ have a default provided. Right now, those options don't get reset when ResetAllOptionOccurrences is called. With this patch, options _without_ cl::init should get reset to the default-constructed value of the option type, and options with cl::init get initialized to the default value.

Wed, Jan 12, 1:44 PM · Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

We have a huge issue with codegen churn causing pages of testcase changes in proposed merges and very little review is done on them.

Can you show an example?

Wed, Jan 12, 10:48 AM · Restricted Project

Tue, Jan 11

greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Agreed with Simon, this is a disaster for doing code review.

Tue, Jan 11, 7:10 PM · Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Have you examples of actual test checks that you've had problems with that this is a answer to?

Tue, Jan 11, 7:06 PM · Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

The implementation looks fine to me, but I’m not sure what the goal is.

Tue, Jan 11, 1:40 PM · Restricted Project
greened added inline comments to D115646: [DAG][TLI][X86][ARM][AArch64] Add `isExtractSubvectorFree` / use it in `foldExtractSubvectorFromShuffleVector()`.
Tue, Jan 11, 1:35 PM · Restricted Project

Mon, Jan 10

greened added a comment to D115433: [CommandLine] Reset option to its default if its Default field is undefined.

What is the motivation for this change? I can argue for maintaining the current behavior., If I pass a cl::init to an option and call ResetAllOptionOccurrences I would expect that cl::init value to be used to reset the option. There are compiler flows that rely on that behavior.

Mon, Jan 10, 8:03 AM · Restricted Project
greened added a comment to D115646: [DAG][TLI][X86][ARM][AArch64] Add `isExtractSubvectorFree` / use it in `foldExtractSubvectorFromShuffleVector()`.

We do need an X86 test for this.

Mon, Jan 10, 7:52 AM · Restricted Project

Fri, Jan 7

greened requested review of D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.
Fri, Jan 7, 11:05 AM · Restricted Project

Dec 2 2021

greened committed rG53adfa8750ea: [clang] Do not duplicate "EnableSplitLTOUnit" module flag (authored by greened).
[clang] Do not duplicate "EnableSplitLTOUnit" module flag
Dec 2 2021, 8:25 AM
greened closed D112177: [clang] Do not duplicate "EnableSplitLTOUnit" module flag.
Dec 2 2021, 8:25 AM · Restricted Project, Restricted Project

Oct 20 2021

greened requested review of D112177: [clang] Do not duplicate "EnableSplitLTOUnit" module flag.
Oct 20 2021, 1:25 PM · Restricted Project, Restricted Project

Jun 23 2021

greened added a comment to D88595: [TableGen] Add not_all_same constraint check.

@paulwalker-arm @sdesmalen I will be picking up this work, I will study this patch and see if I have any questions. Thanks!

Jun 23 2021, 12:00 PM · Restricted Project

Jan 15 2021

greened added inline comments to D88595: [TableGen] Add not_all_same constraint check.
Jan 15 2021, 8:31 AM · Restricted Project

Jan 11 2021

greened added inline comments to D88595: [TableGen] Add not_all_same constraint check.
Jan 11 2021, 8:13 PM · Restricted Project
greened updated the diff for D88595: [TableGen] Add not_all_same constraint check.

Updated to main, implemented some suggested changes.

Jan 11 2021, 7:52 PM · Restricted Project

Dec 15 2020

greened added a comment to D88595: [TableGen] Add not_all_same constraint check.

This is still on my TODO list but got pushed aside for a bit due to other issues. I expect to be back on this in the next couple of weeks.

Dec 15 2020, 6:36 AM · Restricted Project

Oct 8 2020

greened added inline comments to D88595: [TableGen] Add not_all_same constraint check.
Oct 8 2020, 3:14 PM · Restricted Project

Oct 7 2020

greened added inline comments to D88595: [TableGen] Add not_all_same constraint check.
Oct 7 2020, 12:03 PM · Restricted Project

Oct 1 2020

greened added inline comments to D88595: [TableGen] Add not_all_same constraint check.
Oct 1 2020, 7:05 AM · Restricted Project

Sep 30 2020

greened added a comment to D76882: [AMDGPU] Implement CFI for non-kernel functions.

Not my area of expertise but looks fine if the comments from others are addressed.

Sep 30 2020, 2:42 PM · debug-info, Restricted Project
greened added a comment to D76883: [AMDGPU] Implement CFI for CSR spills.

I don't really have the expertise to approve this. The patch looks fine from a readability standpoint.

Sep 30 2020, 2:38 PM · debug-info, Restricted Project
greened added a comment to D78778: [MCAsmInfo] Support UsesCFIForDebug for targets with no exception handling.

This is pretty far afield from my areas of expertise so I can't really comment on the overall approach of this patch. From a purely mechanical/style point of view this seems fine to me.

Sep 30 2020, 2:34 PM · debug-info, Restricted Project
greened added a comment to D68911: [AArch64] enable (v)select to math TLI hook (WIP).

I'm running SPEC CPU intrate for this patch as well as this patch in combination with D78880.

Sep 30 2020, 2:25 PM · Restricted Project
greened added inline comments to D76884: [AMDGPU] Implement -amdgpu-spill-cfi-saved-regs.
Sep 30 2020, 2:10 PM · debug-info, Restricted Project
greened added inline comments to D76880: [AMDGPU] Emit entry function CFI.
Sep 30 2020, 1:33 PM · debug-info, Restricted Project
greened accepted D76879: [AMDGPU] Begin emitting CFI for AMDGCN.
Sep 30 2020, 1:31 PM · debug-info, Restricted Project
greened updated the diff for D88595: [TableGen] Add not_all_same constraint check.

Make clang-tidy (more?) happy.

Sep 30 2020, 1:14 PM · Restricted Project
greened updated the summary of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:22 AM · Restricted Project
greened updated the summary of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:21 AM · Restricted Project
greened updated the summary of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:19 AM · Restricted Project
greened requested review of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:19 AM · Restricted Project

Sep 23 2020

greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

The expensive-checks bots have been red for several days. Could you please take a look or revert?

Working on it...

Sep 23 2020, 9:30 AM · Restricted Project, Restricted Project
greened committed rGb8779337841b: [UpdateTestChecks] Remove bug-exposing test (authored by greened).
[UpdateTestChecks] Remove bug-exposing test
Sep 23 2020, 9:29 AM
greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

The expensive-checks bots have been red for several days. Could you please take a look or revert?

Sep 23 2020, 8:01 AM · Restricted Project, Restricted Project

Sep 18 2020

greened committed rG7c8bb409f31e: [UpdateCCTestChecks] Include generated functions if asked (authored by greened).
[UpdateCCTestChecks] Include generated functions if asked
Sep 18 2020, 4:36 AM
greened closed D83004: [UpdateCCTestChecks] Include generated functions if asked.
Sep 18 2020, 4:35 AM · Restricted Project, Restricted Project

Sep 17 2020

greened updated the diff for D83004: [UpdateCCTestChecks] Include generated functions if asked.

Fix one inadvertent change to the original algorithm in update_test_checks.py. This line:

output_lines.append(input_line)

had an extra rstrip in it compared to the original. It was probably an innocuous change but fixing this makes the diff a bit cleaner.

Sep 17 2020, 2:57 PM · Restricted Project, Restricted Project
greened added inline comments to D83004: [UpdateCCTestChecks] Include generated functions if asked.
Sep 17 2020, 2:52 PM · Restricted Project, Restricted Project
greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

Maybe add another test that checks for the @__cxx_global_var_init() constructor function?
E.g. something like this:

int init_func(int arg) {
    return arg + 1;
}

int my_global = init_func(2);
Sep 17 2020, 2:42 PM · Restricted Project, Restricted Project
greened updated the diff for D83004: [UpdateCCTestChecks] Include generated functions if asked.

Refactored to pull out most of the --include-generated-funcs processing into common.py.

Sep 17 2020, 2:40 PM · Restricted Project, Restricted Project
greened updated the diff for D83004: [UpdateCCTestChecks] Include generated functions if asked.

Enhanced update_test_checks.py to honor --include-generated-funcs and added tests. For fun, did the same for update_llc_test_checks.py. The latter doesn't have complete coverage for all architectures because the machine outliner doesn't run on every backend. I didn't look further into this but we can do so later if desired.

Sep 17 2020, 12:25 PM · Restricted Project, Restricted Project

Sep 16 2020

greened committed rGce0eb81c7274: [UpdateTestChecks] Allow $ in function names (authored by greened).
[UpdateTestChecks] Allow $ in function names
Sep 16 2020, 12:36 PM
greened closed D82995: [UpdateTestChecks] Allow $ in function names.
Sep 16 2020, 12:36 PM · Restricted Project
greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.
Sep 16 2020, 12:26 PM · Restricted Project, Restricted Project
greened updated the diff for D82995: [UpdateTestChecks] Allow $ in function names.

Added tests.

Sep 16 2020, 7:41 AM · Restricted Project

Sep 15 2020

greened added a comment to D82995: [UpdateTestChecks] Allow $ in function names.

@greened Any luck with a test case?

Sep 15 2020, 9:31 AM · Restricted Project
greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

Due to D82995 I realized we should have a test of this in llvm/test/tools/UpdateTestChecks as well.

Sep 15 2020, 9:30 AM · Restricted Project, Restricted Project
greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

reverse ping?

Sep 15 2020, 9:25 AM · Restricted Project, Restricted Project

Jul 22 2020

greened updated the diff for D83004: [UpdateCCTestChecks] Include generated functions if asked.

Updated to move the option into common.py. Also had to rework the output loop to account for differences between python 2 and 3.

Jul 22 2020, 1:26 PM · Restricted Project, Restricted Project
greened added inline comments to D83004: [UpdateCCTestChecks] Include generated functions if asked.
Jul 22 2020, 11:33 AM · Restricted Project, Restricted Project
greened added a comment to D82995: [UpdateTestChecks] Allow $ in function names.

We need a test.

If it helps, OpenMP begin/end declare variant mangles the names with $, so do we for CUDA (sometimes).

Jul 22 2020, 11:29 AM · Restricted Project

Jul 7 2020

greened added inline comments to D83004: [UpdateCCTestChecks] Include generated functions if asked.
Jul 7 2020, 8:01 AM · Restricted Project, Restricted Project

Jul 6 2020

greened added inline comments to D83004: [UpdateCCTestChecks] Include generated functions if asked.
Jul 6 2020, 6:21 PM · Restricted Project, Restricted Project
greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.
Jul 6 2020, 6:18 PM · Restricted Project, Restricted Project
greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

This is great! Just a few days ago I added a TODO in one of the tests I created asking for this: D82722 :)

Jul 6 2020, 2:42 PM · Restricted Project, Restricted Project
greened added inline comments to D83004: [UpdateCCTestChecks] Include generated functions if asked.
Jul 6 2020, 2:38 PM · Restricted Project, Restricted Project
greened updated the diff for D83004: [UpdateCCTestChecks] Include generated functions if asked.

Fixed various bugs, added tests.

Jul 6 2020, 2:34 PM · Restricted Project, Restricted Project
greened added inline comments to D82995: [UpdateTestChecks] Allow $ in function names.
Jul 6 2020, 7:02 AM · Restricted Project
greened added a comment to D82995: [UpdateTestChecks] Allow $ in function names.

It would be good to check in a test example alongside this change, so we know it works. (And we'll know what we are losing if this has to be reverted for some reason.)

Jul 6 2020, 6:56 AM · Restricted Project

Jul 2 2020

greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

Do we have any tests for these tools? I vaguely recall some discussion about tests but I couldn't find any. I'd like to add tests for this feature if the infrastructure is already there.

Yes. See clang/test/utils/update_cc_test_checks/ (thanks to @arichardson)

Jul 2 2020, 10:14 AM · Restricted Project, Restricted Project

Jul 1 2020

greened added a comment to D83004: [UpdateCCTestChecks] Include generated functions if asked.

Do we have any tests for these tools? I vaguely recall some discussion about tests but I couldn't find any. I'd like to add tests for this feature if the infrastructure is already there.

Jul 1 2020, 3:40 PM · Restricted Project, Restricted Project
greened created D83004: [UpdateCCTestChecks] Include generated functions if asked.
Jul 1 2020, 3:40 PM · Restricted Project, Restricted Project
greened created D82995: [UpdateTestChecks] Allow $ in function names.
Jul 1 2020, 2:04 PM · Restricted Project
greened added a comment to D68230: [UpdateCCTestChecks] Allow asm in output with --allow-asm.

Ping?

Jul 1 2020, 1:32 PM · Restricted Project, Restricted Project

Jun 15 2020

greened added a comment to D68230: [UpdateCCTestChecks] Allow asm in output with --allow-asm.
Jun 15 2020, 7:51 PM · Restricted Project, Restricted Project

Jun 8 2020

greened added a comment to D68230: [UpdateCCTestChecks] Allow asm in output with --allow-asm.

Ok, so if test-suite seems.like a good place for such tests, can this change proceed?

Jun 8 2020, 6:16 PM · Restricted Project, Restricted Project

May 22 2020

greened added a comment to D68230: [UpdateCCTestChecks] Allow asm in output with --allow-asm.

In general I have a pretty strong objection to end to end tests in the clang repository unless there's no other way to test them.

Can you give an idea of what you plan on testing with this?

May 22 2020, 9:38 AM · Restricted Project, Restricted Project

May 18 2020

greened added a comment to D68230: [UpdateCCTestChecks] Allow asm in output with --allow-asm.

Getting back to this. From the (very long!) thread on end-to-end testing, people are generally supportive of the idea. The main question is where tests should be located. That question is othogonal to this patch. We currently use the functionality of this patch downstream and I'm guessing others would also find it useful. Since the idea of end-to-end testing seems to be supported in the community, can we merge this patch and then have a follow-up discussion about where such tests should live upstream? Getting this merged now would allow others to experiment with/use the functionality as discussion about where to place end-to-end tests continues.

May 18 2020, 5:52 AM · Restricted Project, Restricted Project

Apr 22 2020

greened added a comment to D78565: [clang][doc] Clang ARM CPU command line argument reference.

Fair enough, perhaps the audience is too small here on llvm.org for this and this is too niche. In A-profile we have the same problem, so could the exercise for an A-core here, but can't spend time on that now, so will abandon this.

I would find this extremely valuable in the clang documentation. Not sure why this should be put on developer.arm.com as it is compiler-specific as wel as (sub-)target-specific.

Apr 22 2020, 10:52 AM
greened added a comment to D78565: [clang][doc] Clang ARM CPU command line argument reference.

Fair enough, perhaps the audience is too small here on llvm.org for this and this is too niche. In A-profile we have the same problem, so could the exercise for an A-core here, but can't spend time on that now, so will abandon this.

Apr 22 2020, 10:52 AM

Mar 2 2020

greened added inline comments to D75385: [TargetLowering] Avoid infinite iteration on setcc fold.
Mar 2 2020, 6:52 AM · Restricted Project

Feb 28 2020

greened added a comment to D75385: [TargetLowering] Avoid infinite iteration on setcc fold.

This appeared using a quite old version of LLVM with an out-of-tree target and I'm having trouble getting a valid testcase out that runs with the latest LLVM/bugpoint. The code in question is exacly the same in the old and current LLVM so the bug still exists currently but I haven't yet been able to generate a testcase. Thought I would at least put this up for review. I'm not entirely confident current master could get into this situation but it seems wise to guard against it.

Feb 28 2020, 2:00 PM · Restricted Project
greened created D75385: [TargetLowering] Avoid infinite iteration on setcc fold.
Feb 28 2020, 2:00 PM · Restricted Project

Feb 18 2020

greened added a comment to D71618: [System Model] Introduce system model classes.
Feb 18 2020, 11:48 AM · Restricted Project
greened added a comment to D68793: [System Model] [TTI] Add TTI interfaces for write-combining buffers.

@jdoerfert @hfinkel should I consider Johannes' comment a LGTM? This has been waiting for two months now.

Feb 18 2020, 11:39 AM · Restricted Project

Feb 3 2020

greened added a comment to D73708: [update_cc_test_checks] Don't attach CHECK lines to function declarations.

LGTM. I did something similar in my local copy that I was going to upstream but I added a switch for the user to choose where to place checks (declaration, definition or both). I only added the switch because I wasn't sure whether the changed behavior was intended. I'm fine just placing checks on definitions as I don't have a need to put them on declarations.

Feb 3 2020, 6:14 PM · Restricted Project
greened accepted D73708: [update_cc_test_checks] Don't attach CHECK lines to function declarations.
Feb 3 2020, 6:14 PM · Restricted Project

Dec 17 2019

greened created D71618: [System Model] Introduce system model classes.
Dec 17 2019, 9:45 AM · Restricted Project
greened added a comment to D69088: [Lex] #pragma clang transform.

This is a major new language feature, and code review is probably not the right venue for reviewing it; there should be a thread on cfe-dev. My apologies if that's already been discussed and I missed it.

Dec 17 2019, 9:37 AM · Restricted Project

Dec 16 2019

greened committed rG055aeb527515: [Bugpoint] Do not create illegal function attribute combos (authored by greened).
[Bugpoint] Do not create illegal function attribute combos
Dec 16 2019, 8:43 AM
greened closed D69522: [Bugpoint] Do not create illegal function attribute combos.
Dec 16 2019, 8:43 AM · Restricted Project
greened updated the diff for D68793: [System Model] [TTI] Add TTI interfaces for write-combining buffers.

Updated to latest master.

Dec 16 2019, 8:43 AM · Restricted Project
greened added a comment to D69534: [X86] Lower frame references with large offsets.

Ping.

Dec 16 2019, 8:06 AM · Restricted Project
greened added a comment to D68793: [System Model] [TTI] Add TTI interfaces for write-combining buffers.

Ping. This is ready for another review.

Dec 16 2019, 7:48 AM · Restricted Project

Dec 3 2019

greened updated the diff for D68793: [System Model] [TTI] Add TTI interfaces for write-combining buffers.

Added comment explaining what a write-combining buffer is and one possibility of how to use this information.

Dec 3 2019, 10:33 AM · Restricted Project
greened added a comment to D68793: [System Model] [TTI] Add TTI interfaces for write-combining buffers.

Ok, I think I've addressed all the comments so this is ready for another review. Thanks!

Dec 3 2019, 10:33 AM · Restricted Project

Nov 1 2019

greened added inline comments to D69534: [X86] Lower frame references with large offsets.
Nov 1 2019, 10:19 AM · Restricted Project