[PATCH] D156026: [InstCombine] Contracting x^2 + 2*x*y + y^2 to (x + y)^2 (integer)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D156026#4525436 <https://reviews.llvm.org/D156026#4525436>, @rainerzufalldererste wrote:

> This includes only the positive tests, not completely sure which specific negative cases you had in mind.

Things like: Shit amount is not 1. Values that must be the same are not the same.

> I'm not entirely sure how to proceed with the single-use stuff

>From the testing side, you will want to pass one/both of the arguments to the final `add` to a `call void @use(i32 %abc)` to add an extra use. On the implementation side, you'll want to use `m_OneUse()`.

> and please let me know how things like those static functions, which are only used in one other function, are handled in LLVM/InstCombine. I've tried storing and reusing the `BinaryOp_match`es themselves, but that's apparently not possible. Would you prefer lambda functions or internal structs with static functions?

Static functions or lambdas are fine. I doubt they are needed here though, if you use commutative matchers.

> Are you happy with relying on || short circuiting, or should that always be a separate if-else? Please let me know :)

Short-circuiting is fine.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1014-1017
+  return match(V, m_Add(m_Mul(m_Specific(A), m_Specific(A)),
+                        m_Mul(m_Specific(B), m_Specific(B)))) ||
+         match(V, m_Add(m_Mul(m_Specific(B), m_Specific(B)),
+                        m_Mul(m_Specific(A), m_Specific(A))));
----------------
Similar in other places. And then drop the static function as unnecessary.


================
Comment at: llvm/test/Transforms/InstCombine/add.ll:3099-3110
+define i32 @add_reduce_sqr_sum_nsw(i32 %0, i32 %1) {
+; CHECK-LABEL: @add_reduce_sqr_sum_nsw(
+; CHECK-NEXT:    [[TMP3:%.*]] = add i32 [[TMP0:%.*]], [[TMP1:%.*]]
+; CHECK-NEXT:    [[TMP4:%.*]] = mul i32 [[TMP3]], [[TMP3]]
+; CHECK-NEXT:    ret i32 [[TMP4]]
+;
+  %3 = mul nsw i32 %0, %0
----------------
Or similar. Avoid unnamed values in tests.


================
Comment at: llvm/test/Transforms/InstCombine/add.ll:3261
+
 declare void @llvm.assume(i1)
----------------
Please separate the test additions into a separate patch, and then base this one on top, so only the test diff is visible. See https://llvm.org/docs/TestingGuide.html#precommit-workflow-for-tests for more information.

This is particularly important to verify that you commutation tests work correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156026