Maybe we can do this in the MachineVerifier too?

LGTM on my side. If this needs to grow into a more generic tool I think we can go with the python bindings and other things at that time.

Thank you!

LGTM

In D112940#3138560, @paquette wrote:

I think it could be generalized though. I think that a really important next step for this is to have a machine-consumable output mode (e.g. JSON) which could have custom views built around it.

Maybe having python bindings that work with bitstream on top of the C API can make this even easier? Going back to a textual format seems wasteful and won't scale when a lot more remarks are enabled.

This is great, thanks! In general everything looks pretty good to me. I'll take a second look while others get some time to comment too!

In D106408#2970470, @rampitec wrote:

In D106408#2970305, @thegameg wrote:

This might be related to https://bugs.llvm.org/show_bug.cgi?id=51655. Do you mind checking it out?

What happened to it? Lldb is green now. Also do you mind to attach ll to the bug? For some reason I cannot build compiler-rt with sanitizers now, still trying to sort it out. This is all not needed to check RA though, if there is an ll.

This might be related to https://bugs.llvm.org/show_bug.cgi?id=51655. Do you mind checking it out?

LGTM

In general I think the syntax looks fine. Not sure I get how hard it is to get rid of the parenthesis, but if we end up changing many tests, we should probably do it only once.

Good catch, thanks!

LGTM on the new constructors.

This looks great! Adding Jessica since she reviewed the original auto-init patches too.

Is there anything you would want to share from llvm/lib/Transforms/Utils/AutoInitRemark.cpp ? You can see some examples in llvm/test/Transforms/Util/trivial-auto-var-init-call.ll.

LGTM with a test.

Flags/attributes/calling conventions look very straightforward.

LGTM, thanks! In general, I like this direction more than the TII::optimizeSelect one.

@JDevlieghere pushing to fix a build failure (https://lab.llvm.org/buildbot/#/builders/149/builds/1934), feel free to still review it!

In D100016#2678789, @skatkov wrote:

@thegameg, I landed NFC before to move from if-else to if-continue. Is it readable now or you still want me to separate added logic into utility function?

In D100017#2673209, @skatkov wrote:

In D100017#2673208, @thegameg wrote:

Is this really useful as a user-surfacing remark? Won't it have a really high false positive rate due to coalescing?

To be honest - not sure. But if RA can assigned physical register and can eliminate copy it a good sign.
So probably I also need to check reg map whether physical registers are different or not... Need to check...

Is this really useful as a user-surfacing remark? Won't it have a really high false positive rate due to coalescing?

This looks good too, thanks! Please add a test or extend the ones in llvm/test/CodeGen/AArch64/arm64-spill-remarks.ll.

In D100012#2673146, @skatkov wrote:

In D100012#2673142, @thegameg wrote:

LGTM. Doesn't seem like there are any tests testing this remark, maybe you can add one if you have something handy?

There are some tests
./CodeGen/AArch64/arm64-spill-remarks-treshold-hotness.ll
./CodeGen/AArch64/arm64-spill-remarks.ll

I'll probably add something like this.

LGTM. Doesn't seem like there are any tests testing this remark, maybe you can add one if you have something handy?

Address comments + comments in header.

Thank you!

Add StoreVolatile: false and StoreAtomic: false under ExtraArgs.

LGTM. Not sure if there's a cleaner way.

LGTM, nice!

LGTM

LGTM

Looks good, thanks!

Looks great, thanks!

In D89892#2349184, @anemet wrote:

How about adding a total count as well? That should help quickly eye ball the ratios.

This looks great! Thanks!

In rG0345d88de654259ae90494bf9b015416e2cccacb#953264, @hgreving wrote:

Is this really necessary to remove? For example, our backend is using EntrySU edges for top edges from the basic block entry, in order to account for clearances. It seems to make sense to have a node like this.

I am totally fine with keeping this in-tree, even unused, if it helps several out-of-tree backends. Thanks for speaking up.

LGTM

Can you wrap the condition around the whole file? The rest is not useful without the library anyway.

Won't be back to this anytime soon.

LGTM

This looks great, thanks! Since it's a CodeGen pass, I would suggest using a MIR test instead of an IR one, but I think it's probably easier to read it as it is right now.

In D85809#2218742, @tejohnson wrote:

Just to clarify, I wasn't suggesting putting it in another patch, but just bringing it back here (plus adding in the gold-plugin support). The other patch I was referring to was the follow on patch D85810 where I also suggested adding support to gold-plugin.cpp.

In D85809#2217341, @weiwang wrote:

I have removed the change to llvm-lto

LGTM, thanks for plugging all these options through!

Hi @compnerd, it looks like this fails to link: http://green.lab.llvm.org/green/job/clang-stage1-RA/13171/console

In D84690#2179205, @dblaikie wrote:

Indeed, sorry that fell through & I didn't understand enough about iterators to instead suggest falling back to an input iterator instead of figuring out how to fit the semantics of the forward iterator.

I had an attempt here: https://reviews.llvm.org/D33419, I think similar concerns came up in that review.

In D84207#2164754, @jroelofs wrote:

Not sure why our public bots didn't catch it.