[PATCH] D70237: [X86] Add more addcarry tests

Paweł Bylica via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 15 01:15:57 PST 2019


chfast added a comment.

In D70237#1745687 <https://reviews.llvm.org/D70237#1745687>, @davezarzycki wrote:

> A few comments / questions:
>
> 1. I'm not a regular contributor to LLVM, so please wait for somebody else (like @craig.topper, @RKSimon, or @spatel) to sign off on this.
> 2. Once one has "add carry", one immediately wants "sub carry/borrow". Please consider adding tests to subcarry.ll.


Yes, I can prepare similar tests for subtraction. Let me know if they should be include in this changeset.

> 3. What is add256_1 trying to do and how is it different than add256_2?

They are now described in comments (also the order has been swapped). Let me know if the descriptions are good enough.

> 4. Finally, can you rename your examples to be more descriptive? In particular, if you look at early parts of the file, you'll see that there are a bunch of definitions with simple names like "add256" where 256 (etc) refers to native LLVM types, not aggregates. Perhaps names like the tests I added "sub_U320_without_i128_or" which strongly implies at the implementation strategy.

Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70237/new/

https://reviews.llvm.org/D70237





More information about the llvm-commits mailing list