@rjmccall would you be able to review this patch? What do you think of this approach? This change is to support C++-Interop on the Swift side.

In D86049#2844766, @dmaclach wrote:

Has anything happened with this at all or did it get dropped?

LGTM once you've addressed @phosek's nits.

I will take another look over this commit in the morning. Overall looks good.

@jkorous thoughts?

This makes sense to me. I approve. Can we move the 3/60 seconds number to a const int value set somewhere higher up in the file as a global with a comment explaining this as well?

If @compnerd has no gripes go ahead and land. LGTM.
Before landing, please make sure to run the check-clang tests as well because the interface stubs generation on the clang driver side invokes llvm-ifs.

Ping? Haven't seen any chatter in a while. Anything new here? I don't want to hold you folks up.

@wolfgangp I get this warning on macOS. I noticed it wasn't fixed in https://github.com/llvm/llvm-project/commit/c6e8f81410a2942b5abd112aa6e468268e01d946#diff-67f7be4aff998787c9c4efcb95e5d0dc225e1dcd37b9b005a3db5493f02efdc3 so I posted this small diff.

Is this still open? Seems good to go.

In D94461#2494710, @compnerd wrote:

You mean use armv7---elf as a triple? While that is possible, keep in mind that it might change the ABI of the generated interfaces. While it is unlikely to matter as the ifso cannot be used at runtime, if the linker attempts to do something like symbolic resolution of ABI dependent specialization (e.g. strcmp.eabi vs strcmp where the latter is generic), that could matter. However, adding support to explicitly override the encoded triple based on the user's input is reasonable (though might deserve a warning that the triple is being overridden).

In D94461#2494667, @compnerd wrote:

The full triple is needed in some cases: armv7-unknown-linux-gnueabi vs armv7-unknown-linux-gnueabihf vs armv7-unknown-linux-eabihf vs armv7-unknown-linux-eabi are all different. Additionally, there is nothing that prevents you from doing something like: armv7-unknown-linux-gnueabihf-coff or armv7-unknown-linux-gnueabihf-macho to generate COFF or MachO object files that are reading in additional bits of ABI information and the libc from the target triple. I really think that preserving the target triple is best.

In D94461#2492369, @phosek wrote:

In D94461#2491947, @plotfi wrote:

In D94461#2491946, @plotfi wrote:

1. I think for unification, the first thing would be unifying the text abi file format. Currently, the YAML format used by ifs is very similar to the one used in InterfaceStub(elfabi), however there are not compatible. One thing is that ifs embeds llvm triples in the text ifs file while elfabi does not do that. Elfabi get platform and endianness information from command line instead of getting them from the text stub file. We did that because there are use cases that we generate ELF stubs for multiple platforms using a single set of text abi files. Is there a specific reason why ifs text files require llvm triples? Is it feasible to make llvm-ifs to get the platform information from the command line options?

The triple-ness was added because originally IFS was intended to be a tool that the clang driver invokes. The idea was that you build some project with IFS enabled and you automatically get the stubs for shipping or for CI purposes to track what new exposure was done in recent builds of said project. I think it would be ok to add a flag to override the triple if that isn't there already. @compnerd
thoughts?

I meant to add, as part of llvm-ifs being invoked by clang the idea was that clang would just forward its triple and llvm-ifs would decide what to do with it because we wanted the ifs format to be kinda platform agnostic but we wanted llvm-ifs to potentially be able to generate stubs for multiple platforms in the future (like MS .libs and tbds etc too).

Would you need a full triple in that case? Other binary tools like linkers doesn't use a full triple either, they use just e_machine and e_ident. Would that be sufficient?

In D94461#2491946, @plotfi wrote:

1. I think for unification, the first thing would be unifying the text abi file format. Currently, the YAML format used by ifs is very similar to the one used in InterfaceStub(elfabi), however there are not compatible. One thing is that ifs embeds llvm triples in the text ifs file while elfabi does not do that. Elfabi get platform and endianness information from command line instead of getting them from the text stub file. We did that because there are use cases that we generate ELF stubs for multiple platforms using a single set of text abi files. Is there a specific reason why ifs text files require llvm triples? Is it feasible to make llvm-ifs to get the platform information from the command line options?

The triple-ness was added because originally IFS was intended to be a tool that the clang driver invokes. The idea was that you build some project with IFS enabled and you automatically get the stubs for shipping or for CI purposes to track what new exposure was done in recent builds of said project. I think it would be ok to add a flag to override the triple if that isn't there already. @compnerd
thoughts?

1. I think for unification, the first thing would be unifying the text abi file format. Currently, the YAML format used by ifs is very similar to the one used in InterfaceStub(elfabi), however there are not compatible. One thing is that ifs embeds llvm triples in the text ifs file while elfabi does not do that. Elfabi get platform and endianness information from command line instead of getting them from the text stub file. We did that because there are use cases that we generate ELF stubs for multiple platforms using a single set of text abi files. Is there a specific reason why ifs text files require llvm triples? Is it feasible to make llvm-ifs to get the platform information from the command line options?

Updated based on @paquette's feedback. This only includes the StableHashTree data structure and a unit test.

Nice. Will you folks be updating llvm-ifs to use this support instead of yaml2obj soon? If so add me to the review, I will be happy to take a look.

Thanks for fixing this @cishida!

I think I am overall ok with this for now until the TBE folks post some more patches to integrate ifs and tbe.

@vsk any update on this? Is there anything we can do to help in landing this patch in llvm-project/main?

Overall looks good.

comments added

clang-tidy

spelling and grammar

Cleaning up patch to be easier to understand.

@sdesmalen @efriedma @dmgreen Hi Sander, Eli, Dave. Would any of you have some time to help review this Prolog Epilog Size optimization patch? Much appreciated if you do. Me and @kyulee would be available to chat on IRC or discord about it for more info.

@thegameg: @paquette tells be you might have some ideas on a better format than this json business going on here (based on work you've done on remarks). What do you think?

@mpaszkowski ping

In D86049#2253705, @plotfi wrote:

I've updated the diff to reflect the alternate non-wrapper exposure. This way requires the sanitizing of the exported objc_direct method name.

In D86952#2254880, @paquette wrote:

Oh wait the tests already test what I was concerned about. LGTM with minor nits.

(although decoupling the tests from mir-canon would be nice, since this is supposed to be standalone)

In D86952#2254859, @paquette wrote:

Is there any way to test the hash itself on certain instructions?

e.g. verify that two instructions with different address spaces get a different hash?

I've updated the diff to reflect the alternate non-wrapper exposure. This way requires the sanitizing of the exported objc_direct method name.

@YangZhihui landed at https://github.com/llvm/llvm-project/commit/f22d27624b6532a5542b283de9ce586c72c6b846

In D86427#2235630, @YangZhihui wrote:

how can I add the Author property in the patch
Thanks

In D86427#2235574, @YangZhihui wrote:

Thanks, but I don't have commit access

LGTM. Go ahead and land it.

@lanza I did it as a CodeGen option for now because we don't want anything like this to be the default until the ABI is fleshed out.
I think one danger in altering the name of the function to some extent is you dont want to clash potentially with user defined C functions.

Ok, LGTM. I will land for you, but in your name on git. Thanks for this patch @kyulee.

In D86049#2225202, @rjmccall wrote:

I think that, if we want to do this, we need to think carefully about what exactly we want the ABI to be.

change for clang-tidy and clang-format.

I think this LGTM. @kyulee are there any other tests that use this flag? Can you add a test that both adds and also removes some different attributes. Also a test that adds and removes he same attribute too would be good.

Good stuff. Thanks for replying so quickly @fhahn :-)

landed at https://github.com/llvm/llvm-project/commit/33490acf241a487028bfee73c0bd9e8026fcdb5c

@fhahn I ran into an llvm_unreachable when building matrix-types-spec.cpp as part of an llvm-test-suite build using -Oz with -flto=thin. I reduced the IR that hit the unreachable to:

Please Update Diff.

In D85678#2211933, @phosek wrote:

I'd be fine with InterfaceStub as a name for the library.

In D85678#2211904, @haowei wrote:

In D85678#2211863, @MaskRay wrote:

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust.

How is it not robust?

We contacted plotfi and learned that llvm-ifs does not work well in production when it is used for elf stub generation. We suggested to improve llvm-ifs tools and unify it with llvm-elfabi tool as they share many similarities. llvm-elfabi has its own elf handler, most of its code have not yet landed and we will try our best to make it to production grade. This CL is the first step to strip elf handler away from llvm-elfabi and put it into a standalone library so we can continue work on it and make it shared by both tools without breaking anything.