[PATCH] D70169: [ConstantFold] Handle identity folds at top of ConstantFoldBinaryInst

Wed Nov 13 05:30:04 PST 2019

spatel added inline comments.

================
Comment at: llvm/lib/IR/ConstantFold.cpp:992
+    // FIXME: remove unnecessary duplicated identity patterns below.
+  Constant *Identity = ConstantExpr::getBinOpIdentity(Opcode, C1->getType());
+  if (Identity) {
----------------
Add a code comment for future reference? An identity constant always allows returning the other operand including undef/poison.
There's some possibly related discussion about that in:
https://bugs.llvm.org/show_bug.cgi?id=43958

Also, we can handle several more identity patterns like:
 // X << 0 = X
by passing the optional "AllowRHSConstant" parameter to getBinOpIdentity().

So that could be another TODO for a follow-up patch. Our test coverage seems very lacking for this function though...so that should be improved first to make sure we're not breaking any rules.

================
Comment at: llvm/test/Transforms/InstCombine/binop-identity-undef.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -instcombine -S %s | FileCheck %s

----------------
This file would be better placed under test/Analysis/ConstantFolding and RUN with -constprop. (Don't need full instcombine or even instsimplify to get these transforms.)

================
Comment at: llvm/test/Transforms/InstCombine/vec_shuffle.ll:1037

 ; FIXME: We should be able to move the AND across the shuffle, as -1 (AND identity value) is used for undef lanes.
 define <4 x i16> @and_constant_mask_undef_3(<4 x i16> %add) {
----------------
Here and similarly below - remove and/or adjust the comment line since we handle this now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70169