Page MenuHomePhabricator

greened (David Greene)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Feb 9 2022

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

I think my concerns still exist, even though you provide one more option. The author selects between cannot foresee what in their tests will be affected by future commit. And the --scrub-reg deps loses too much information.

Feb 9 2022, 3:48 PM · Restricted Project

Feb 7 2022

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

I guess to me this just brings feature parity with update_test_checks.py to update_llc_test_checks.py. Both can now scrub variable/register names.

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

I'm still struggling to understand real use cases here - you spoke about just wanting to match specific instructions (nontemporal load/store) - but wasn't that handled by D117694 --filter and --filter-out?

I guess I'm getting worried that these scripts are experiencing feature creep for use cases that are barely a real issue.

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

Ping. I think the latest changes should address some of the concerns about applicability. The scrub-regs=deps option creates a very generic kind of check.

Feb 7 2022, 8:19 AM · Restricted Project

Jan 31 2022

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

Updated to change to a --scrub-reg option with a value of "names" or "deps." --scrub-reg=names will create FileCheck variables as before while --scrub-reg=deps will replace with non-capturing regular expressions.

Jan 31 2022, 1:27 PM · Restricted Project
greened committed rGecd46edd6134: [UpdateTestChecks] Re-add --filter and --filter-out options (authored by greened).
[UpdateTestChecks] Re-add --filter and --filter-out options
Jan 31 2022, 1:12 PM
greened updated the diff for D117694: [UpdateTestChecks] Add --filter and --filter-out options.

Updated with bug fixes. Posting so that future visitors will see the change mentioned in the Differential Revision field of the commit message.

Jan 31 2022, 1:10 PM · Restricted Project, Restricted Project

Jan 28 2022

greened added a reverting change for rG030f71698d52: [UpdateTestChecks] Add --filter and --filter-out options: rG7e32d2b21a58: Revert "[UpdateTestChecks] Add --filter and --filter-out options".
Jan 28 2022, 5:08 PM
greened committed rG7e32d2b21a58: Revert "[UpdateTestChecks] Add --filter and --filter-out options" (authored by greened).
Revert "[UpdateTestChecks] Add --filter and --filter-out options"
Jan 28 2022, 5:08 PM
greened added a reverting change for D117694: [UpdateTestChecks] Add --filter and --filter-out options: rG7e32d2b21a58: Revert "[UpdateTestChecks] Add --filter and --filter-out options".
Jan 28 2022, 5:07 PM · Restricted Project, Restricted Project
greened committed rG030f71698d52: [UpdateTestChecks] Add --filter and --filter-out options (authored by greened).
[UpdateTestChecks] Add --filter and --filter-out options
Jan 28 2022, 2:11 PM
greened closed D117694: [UpdateTestChecks] Add --filter and --filter-out options.
Jan 28 2022, 2:11 PM · Restricted Project, Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

Ping.

Jan 28 2022, 9:26 AM · Restricted Project
greened added a comment to D117694: [UpdateTestChecks] Add --filter and --filter-out options.

Ping. Can we merge this?

Jan 28 2022, 9:24 AM · Restricted Project, Restricted Project

Jan 21 2022

greened updated the diff for D117694: [UpdateTestChecks] Add --filter and --filter-out options.

Fixed a brown paper bag bug (filter-out was not working).

Jan 21 2022, 2:53 PM · Restricted Project, Restricted Project
greened updated the diff for D117694: [UpdateTestChecks] Add --filter and --filter-out options.

Fixed typo.

Jan 21 2022, 2:07 PM · Restricted Project, Restricted Project
greened added inline comments to D117694: [UpdateTestChecks] Add --filter and --filter-out options.
Jan 21 2022, 1:46 PM · Restricted Project, Restricted Project
greened added a comment to D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.

What I mainly wanted to demonstrate is there exists such a test generated for other purpose. For a scheduling patch, the unexpected changes of other use and def of %R1 and %R2 on an unrelated test are confusing.

; CHECK-NEXT:    add %[[GPR_1:(%r[0-9]+)]], %[[GPR_2:(%r[0-9]+)]] <== These two lines won't be changed, that's fine.
; CHECK-NEXT:    add %[[GPR_1:(%r[0-9]+)]], %[[GPR_3:(%r[0-9]+)]]
; ... ...
; CHECK-NEXT:    other_use_def %[[GPR_2]] <== These lines will be changed, that's bad.
; CHECK-NEXT:    other_use_def %[[GPR_3]] <== Same here.
Jan 21 2022, 1:44 PM · Restricted Project

Jan 19 2022

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.

Jan 19 2022, 9:41 AM · Restricted Project
greened requested review of D117694: [UpdateTestChecks] Add --filter and --filter-out options.
Jan 19 2022, 9:40 AM · Restricted Project, 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.

Jan 19 2022, 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?

Jan 19 2022, 7:04 AM · Restricted Project

Jan 18 2022

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.

Jan 18 2022, 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.

Jan 18 2022, 8:49 AM · Restricted Project

Jan 14 2022

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

Clarified comments.

Jan 14 2022, 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.

Jan 14 2022, 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.

Jan 14 2022, 8:21 AM · Restricted Project

Jan 13 2022

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.

Jan 13 2022, 8:05 AM · Restricted Project

Jan 12 2022

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?

Jan 12 2022, 2:31 PM · Restricted Project
greened added inline comments to D98230: [LSR] Add reconciliation of unfoldable offsets.
Jan 12 2022, 1:53 PM · Restricted Project, 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.

Jan 12 2022, 1:44 PM · Restricted Project, 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?

Jan 12 2022, 10:48 AM · Restricted Project

Jan 11 2022

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

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

Jan 11 2022, 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?

Jan 11 2022, 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.

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

Jan 10 2022

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.

Jan 10 2022, 8:03 AM · Restricted Project, 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.

Jan 10 2022, 7:52 AM · Restricted Project

Jan 7 2022

greened requested review of D116832: [UpdateLLCTestChecks] Allow replacing register names with variables.
Jan 7 2022, 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, 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, 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, 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, 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, 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, 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, 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, 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, Restricted Project
greened updated the summary of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:22 AM · Restricted Project, Restricted Project
greened updated the summary of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:21 AM · Restricted Project, Restricted Project
greened updated the summary of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:19 AM · Restricted Project, Restricted Project
greened requested review of D88595: [TableGen] Add not_all_same constraint check.
Sep 30 2020, 10:19 AM · Restricted Project, 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