[PATCH] D156449: [LLVM][Transforms] Zext flag in various optimization passes for RISC-V

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:383
+    if (auto *ZExtI = dyn_cast<ZExtInst>(NewExt))
+      ZExtI->setWasSext(true);
     return BinaryOperator::Create(I.getOpcode(), Op0, NewExt);
----------------
I don't think this is correct if we want the semantics of `was_sext` to be that the input to the zext has its sign bit clear.

This transform is based on the consuming instruction rather than anything about the producer.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1269
           Value *NewLShr = Builder.CreateLShr(X, SrcTyBitWidth - 1);
-          return new ZExtInst(NewLShr, Ty);
+          auto *ZExt = new ZExtInst(NewLShr, Ty);
+          ZExt->setWasSext(true);
----------------
This change seems unnecessary. computeKnownBits should already be able to prove that lshr produces a positive number.

If we want to have this flag explicitly set anytime the sign bit of the input is 0, we should call isKnownNonNegative from visitZExt instead.


================
Comment at: llvm/lib/Transforms/Scalar/BDCE.cpp:124
+            Builder.CreateZExt(SE->getOperand(0), DstTy, SE->getName());
+        if (auto *ZExtI = dyn_cast<ZExtInst>(ZExt))
+          ZExtI->setWasSext(true);
----------------
This is based on the consumer. That doesn't match the semantic we're trying to have of this bit giving information about the input.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:552
       Pred = ICmpInst::getUnsignedPredicate(Pred);
+      Ext->setWasSext(true);
     } else {
----------------
I don't think this is correct for all cases that CanUseZExt returns true. Many of the cases are based on the consumer rather than the producer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156449