Ping

Ping

A patch adding the same functionality was added back in May: D61719, rL360381.

Ping

LGTM

Ping

This makes the IR self-contained, which is good, but it also make the interpretation of the IR modal, which isn't great. It means that suddenly the rules of interpretation of what is valid to do or not changes according to this module flag.

LGTM

Ping

Ping.

LGTM

LGTM

Ping

LGTM

This patch looks good, but presumably needs the clang half to be committed at the same time?

• Limit to minsize (-Oz)
• Count common instructions at the start of the true/false blocks
• Non-analyzable branches and branches in forked diamonds will be merged, not removed

LGTM

LGTM

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

Does the name of the -falways-need-cfi option match any existing compiler (google doesn't find anything)? If not, I think it would make more sense as a -g option, as it's similar to -gline-tables-only.

Test?

LGTM with one small change.

Yep, I can commit it for you.

LGTM

LGTM with one style nit.

LGTM

LGTM with one small nit.

It isn't that common, but it seems worth doing if it can be done easily.

LGTM, thanks.

LGTM

LGTM

LGTM with a minor style nit.

LGTM, thanks

LGTM

Hi Sam, this patch seems to have caused a correctness regression in the telecomm-gsm and toast benchmarks when compiled for Arm. Logs at http://lab.llvm.org:8011/builders/clang-native-arm-lnt-perf/builds/170/. Would you mind looking into this?

LGTM

LGTM

LGTM

• Switch to using ArrayRef instead of null-terminated array

LGTM, thanks

Ping.

Ping.

• Add LTOPostLink metadata, instead of internalising vcall visibility at LTO time

Partial linking will indeed prevent dropping the virtual functions, but it should not prevent clearing the pointer to the virtual function in the vtable. The linker should then be able to drop the virtual function body as part of --gc-sections during the final link.

LGTM

• Take the example from my earlier message, give the "main executable" and "DSO" hidden visibility, build the "main executable" with LTO and the "DSO" without LTO, and statically link them both into the same executable. We run into the same problem where the Plugin1 vtable is potentially not referenced and thus misoptimised. Yes, this is a violation of the LTO visibility rules, but the example shows that we only detect it sometimes. I think that if we did want to detect cases where the LTO visibility rules are clearly being violated, the outcome should be to issue a diagnostic and not to silently proceed with optimizations disabled, since the violation might be masking other undetected issues. That really seems orthogonal to this patch, though.

LGTM, thanks

LGTM

LGTM

LGTM

• Rebase
• Don't emit llvm.assume when not necessary (we already weren't checking for it's presence in GlobalDCE)
• s/"public"/"default"/ in IR docs

Out of interest, why does the ABI allow functions which don't have SVE args/returns to clobber the P registers? For Z registers, we've got to be compatible with old code which only needed to save the bottom half of v8-v15, but there should be no existing code which uses P registers, so we could enforce a mixture of callee- and caller-saved P registers for all code. Existing code is already compliant with this, because it doesn't touch the P regs.

In that example, with everything having default ELF visibility, all of the vtables will get vcall_visibility public, which can't be optimised by VFE, and won't ever be relaxed to one of the tighter visibilities.

Would this also be profitable for Thumb2?

Ok, but please add a comment, probably in AArch64.td, explaining this, and that turning it on by default is only temporary.

Ping

Ping

This patch LGTM now, and +1 to the idea of documenting the attacks we do/don't expect to be able to defend against.