[PATCH] D146928: [NaryReassociate] Transform expression (X << C1) + C2 to (X + C3) << C1,

Thu Mar 30 15:55:07 PDT 2023

AlexM planned changes to this revision.
AlexM added a comment.

In D146928#4232813 <https://reviews.llvm.org/D146928#4232813>, @mkazantsev wrote:

> Do you know why at all InstCombine would want to transform `(X + const1) << 22` into `(X << 22 + const2)`?

This functionality was added in 44bd392cbff238e75d84ee757189d5f62cf91679 which says simply that it helps by "opening opportunities for future improvements."

In D146928#4232900 <https://reviews.llvm.org/D146928#4232900>, @ebrevnov wrote:

> As I see it, you are trying to solve pass ordering issue (InstCombine prevents GVN/CSE). But your solution assumes there is GVN/CSE follows NaryReassociation what creates another potential pass ordering dependence.
> Another point is the way the transformation is implemented right now doesn't fit intent of NaryReassociation pass. In the heart of NaryReassociation  pass is findClosestMatchingDominator call to find and delete common expressions after reassociation. Since you are not checking for existence of common expressions you can easily do the transformation in some other place (ordinary Reassociate pass, or InstSimplify/InstCombine)
> In order to solve both mentioned problems and cover your case you will need to use findClosestMatchingDominator and possibly extend it to look for post dominating expressions as well.

Hmm, this is a pretty fundamental issue, let me take some time to figure out how I can integrate this more deeply into the pass or move it somewhere more appropriate.

================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:510
+    auto *C3Val = ConstantInt::get(I->getType(), C2->lshr(*C1));
+    auto *Add = BinaryOperator::CreateAdd(X, C3Val, "add.nary", I);
+    auto *C1Val = ConstantInt::get(I->getType(), *C1);
----------------
mkazantsev wrote:
> If the initial operations had `nuw/nsw` flags, will they now be lost? Can we preserve them somehow?
This dose seem like an issue, might require a bit more code but certainly seems feasible. I did check InstCombine, which does the opposite transformation and these flags are completely discarded there. I'm very unfamiliar with the semantics of these flags but if dropping them is a problem here is it also a problem in InstCombine? Is dropping them a correctness issue or does it potentially prevent other optimizations?

================
Comment at: llvm/test/Transforms/NaryReassociate/NVPTX/const-shl.ll:1
+; RUN: opt < %s -passes=instcombine,nary-reassociate -S | FileCheck %s
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s --check-prefix=INSTCOMB
----------------
mkazantsev wrote:
> Could you please precommit the test to show what your patch is changing?
This is an embarrassingly ignorant question but what do you mean by this?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146928