• Fixing clang-format

• Adding back unrelated file to please the Merge bot

• remove wrongly added file

• Rebase and fix broken unit tests

Thx!

@efriedma does it look good enough as is? Any other suggestions?

• Add instructions for frequency scaling
• Add RATIONALE.md file

• Be explicit about Python3 usage

In D72516#1824779, @sivachandra wrote:

This change is fine with me. I will let @ckennelly approve it. A nit inline, and also, is RATIONALE.md still missing?

In D72516#1822781, @sivachandra wrote:

I did one detailed pass. I don't claim to understand the code fully but in general I do not see anything popping out as a big problem. I left a few inline comments, but an overall comment is that there are style violations at many places. @abrachet pointed out few of them, but I see there are a number of others as well. Especially, the variable naming convention follows google-style and not LLVM-style.

• Fix variable case

• Add missing headers
• Split benchmarking targets into render and display
• Fix missing return value warning
• Make small configuration run up to 1KiB
• Change window position in graph
• Add README with quickstart
• Add missing newline
• Remove trailing _ in header guard
• Use quotes for includes
• Rename cc files to cpp
• Remove *- C++ -* in cpp files
• Change filename case
• Reorder include
• Fix test error
• Fix JSON comments and tests
• Fix missing comments trailing dot
• Fix enum case
• Moved NDEBUG warning to main
• Removed unneeded inline keyword
• Remove function pointer dereferencing
• Used regular EXPECT wherever possible
• Remove heading whitespace
• Format README.md to respect 80 columns
• Use SUITE instead of custom test target
• anchor memcpy

In D71710#1822800, @efriedma wrote:

In D71710#1821546, @gchatelet wrote:

In D71710#1806641, @efriedma wrote:

It would be nice to do a bit more work so various optimizations handle llvm.memcpy.inline in a way that isn't completely conservative (for example, in alias analysis), but I guess that doesn't need to be in the initial patch.

I'm not too familiar with AA but my understanding is that the definition in llvm/include/llvm/IR/Intrinsics.td already describes the arguments with enough precision to be helpful for the AA pass.

def int_memcpy_inline
    : Intrinsic<[],
      [ llvm_anyptr_ty, llvm_anyptr_ty, llvm_anyint_ty, llvm_i1_ty ],
      [ IntrArgMemOnly, NoCapture<0>, NoCapture<1>, WriteOnly<0>, ReadOnly<1>,
      ImmArg<2>, ImmArg<3> ]>;

Did you have something specific in mind? Or am I missing something completely?

The attributes are helpful, but not enough to figure out how many bytes are modified.

• Simplify code

In D71710#1806641, @efriedma wrote:

It would be nice to do a bit more work so various optimizations handle llvm.memcpy.inline in a way that isn't completely conservative (for example, in alias analysis), but I guess that doesn't need to be in the initial patch.

• Rebase and address comments

Removing myself from the reviewers (I'm not an owner of this code)

In D60000#1785687, @gchatelet wrote:

In D60000#1785210, @lebedev.ri wrote:

@courbet @gchatelet bump. any plans on working on that functionality anytime soon? :)
Not sure how healthy for any project such silence in form of neither working on
nor allowing other interested parties to work on missing functionality...

Yes I'll be working on it from now on. Stay tuned.

• Add missing headers
  • Split benchmarking targets into render and display
  • Fix missing return value warning
  • Make small configuration run up to 1KiB
  • Change window position in graph
  • Add README with quickstart
  • Add missing newline
  • Remove trailing _ in header guard
  • Use quotes for includes
  • Rename cc files to cpp
  • Remove *- C++ -* in cpp files
  • Change filename case
  • Reorder include
  • Fix test error
  • Fix JSON comments and tests
  • Fix missing comments trailing dot
  • Fix enum case
  • Moved NDEBUG warning to main
  • Removed unneeded inline keyword
  • Remove function pointer dereferencing
  • Used regular EXPECT wherever possible
  • Remove heading whitespace

• Remove heading whitespace

• Fix JSON comments and tests
• Fix missing comments trailing dot
• Fix enum case
• Moved NDEBUG warning to main
• Removed unneeded inline keyword
• Remove function pointer dereferencing
• Used regular EXPECT wherever possible

• Add missing newline
• Remove trailing _ in header guard
• Use quotes for includes
• Rename cc files to cpp
• Remove *- C++ -* in cpp files
• Change filename case
• Reorder include
• Fix test error

• Split benchmarking targets into render and display
• Fix missing return value warning
• Make small configuration run up to 1KiB
• Change window position in graph
• Add README with quickstart

In D72516#1816264, @sivachandra wrote:

I notice you have a TODO to add a README.md. But, I think some doc like that would really help review this better.

In D72516#1814843, @abrachet wrote:

This is a very big patch and I don't have too much time today so I haven't reviewed this all and I focused on llvm style for now. A lot of comments I only made once but apply throughout.

I know something was posted on the mailing list today and I'm guessing this was talked about offline before work started here but I wish this was done more incrementally and discussed on the lists before. This is pretty hard to review as it is.

• Add missing headers

Thx for the summary @tejohnson.

Thx !

• Fixed wrong description for the header

• Address comments

Can someone else have a look and approve this patch?

• Fix newline + remove =0

Thx !

• - Add a Lint test for overlapping buffers

In D71710#1791589, @jdoerfert wrote:

This looks pretty reasonable to me. Test coverage is low though:
I think we need to add it to the read/write intrinsic tests (that I thought existed). Also some tests that verify we handle memcpy in a certain way should be copied for this intrinsic.

• address comments

• Address comments

In D71547#1786175, @aaron.ballman wrote:

When I apply locally and build, I get no additional warnings so this LGTM. Thanks!

• rebase to get the correct diff

In D60000#1785210, @lebedev.ri wrote:

@courbet @gchatelet bump. any plans on working on that functionality anytime soon? :)
Not sure how healthy for any project such silence in form of neither working on
nor allowing other interested parties to work on missing functionality...

• Add some documentation

• Address comments

Same as @courbet, I know almost nothing about MIPS but LGTM

Thx !

Thx !

I've created D71468 which should fix the issue for all backend at once and prevent hacking in MipsAsmParser.cpp

In D71213#1783467, @nhaehnle wrote:

In D71213#1781322, @gchatelet wrote:

LLVM has this LLVM_ATTRIBUTE_DEPRECATED macro, it's convenient to get a warning but it only works when building without -Wall.

Did you mean to write _with_ -Wall? I fail to see anything that stops the macro from working with -Wall?

Generally, I'd say using that macro a bit more for this kind of thing would be a good idea.

In D71137#1781876, @mstojanovic wrote:

@gchatelet @courbet How do you think the example I mentioned in the previous comment should be handeled on X86? This might inform us what (if anything) should be changed for MIPS.

@foad do you have any insights on how to go with the deprecation?
LLVM has this LLVM_ATTRIBUTE_DEPRECATED macro, it's convenient to get a warning but it only works when building without -Wall.

Thx for letting me know @foad . I'll make sure to keep the old API with a deprecation message from now on.
Do you have any other suggestions on how to make this less painful for out-of-tree users? I'm afraid that the cleanup phase (removal of deprecated function) will be disruptive as well.

Removing deprecated functions is generally OK, as long as it happens *after* the preferred function is introduced, so we have time to switch over.

In D71213#1779841, @foad wrote:

@gchatelet in general would it be possible to make changes like this in a backwards-compatible way, or in two stages without a "flag day" change? We have out-of-tree users of CreateMemSet and it's awkward to change them all at exactly the same time as we merge in this change from upstream llvm, and we have had the same problem with other MaybeAlign changes recently. I realise that LLVM doesn't make any official promises about API stability.

Thx