I also think TableSEH is a bit clearer. And yea, I'll try to clean up other similar Win64 cases as I get around.

In D89622#2340812, @compnerd wrote:

Thanks for doing this!

Nit: consolidate getRegisterNames calls.

Address review comments.

Accidentally messed up the last diff there. Update revision with final code change that was committed for visibility.

Address nit: Use early return to reduce indentation.

Mask out existing alignment bits.

Ping?

Address review comments.

In D88635#2316128, @grimar wrote:

I think you can:

1. Merge 7 test cases you have into one (use yaml2obj's --docnum option, see examples in llvm-readobj\ELF\)
2. Perhaps you can reuse YAMLs with use of yaml2obj's macros feature (-D<macro>=<value>). See llvm-readobj\ELF\reloc-addends.test and many other tests.
3. It feels that you should be able to reduce YAML descriptions, e.g. remove excessive sections? (so, try to keep parts that are related/needed only)

• Consolidate and simplify tests.

Update tests.

Add 32bit test as well.

Add more tests and use YAML instead of precompiled binary.

Update.

Update.

Update.

Use the right flag.

In D88635#2305377, @jhenderson wrote:

Thanks for the patch. It looks to me like this patch is intended to add a new option to llvm-readobj. This should be tested in the llvm-readobj tests, and not in the LLD tests. If you want a new test for LLD, you'll probably want a separate patch for using this new option. You'll want to use a different method to create your input for llvm-readobj, since the LLVM tools should be testable without it. This might require you to use something like yaml2obj to create your test input (I'm assuming you need a "linked" image, rather than a relocatable COFF object - I'm not particularly familiar with COFF).

You also need to update the documentation located at llvm\docs\CommandGuide\llvm-readobj.rst with the new option.

I can't comment on the functional logic of the patch, due to lack of expertise with COFF. Please make sure to add some reviewers who have touched/reviewed the COFF dumping code recently.

Add newline at the end of new test.

Add llvm-readobj test and address comments.

Rebase over committed baseline tests.

In D60036#1454548, @nikic wrote:

We should probably give reenabling the option by default another try (in a separate revision), at least I can't seem to reproduce the issue in PR31181 anymore.

Rename option and method to be more general.

Ping?

Rebased on committed tests.

Give tests more descriptive names and add one more negative test.

In D59916#1445590, @nikic wrote:

Can you please split off the InstCombine change into a separate revision?

Split off CorrelatedValuePropagation changes to new revision: D60036

In D27114#707685, @efriedma wrote:

LGTM.

Luqman, thanks for putting up with the long delay in reviewing this.

Do you have commit access, or should I commit this for you?

I added another test for all the relevant codepaths in PromoteMemoryToRegister.
I also addressed one case I missed earlier: when the alloca in question has
multiple stores but all within the same basic block.

Ping?

Use copyMetadata method instead of manually copying metadata.

Ping

Ping?

Don't add assume if the value is already known to be nonnull.

Ping

Ping

Ping

So at least running the test suite I didn't come across any performance regressions. Could we land this and observe the buildbots or something for any regressions and back out as necessary?