[PATCH] D140858: [InstCombine]: Don't simplify bits if it causes imm32 to become imm64

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline for some code comment adjustments.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1725
     // Set high zeros of C2 to allow matching negated power-of-2.
-    NewC2 = *C2 + APInt::getHighBitsSet(C2->getBitWidth(),
                                         Know.countMinLeadingZeros());
----------------
Adding is a bug, but I can't find a way to expose it since we always call SimplifyDemandedBits before we reach here. It's possible this patch will expose other bugs like that, so we'll need to watch for fallout.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:47-53
+  // Many targets have optimizations for encoding constants of a repeated
+  // pattern. Sign extension is one of the more trivial/common ones so
+  // avoid breaking it. For example on X86 this can result in breaking imm8
+  // encoding or cause an extra `movabs` instruction, On AArch64 it can cause
+  // `ldq` or `movk + lsl` to be emitted instead of a recognized immediate
+  // pattern. So if `ShrunkC` will require a larger sign-extended type than `C`
+  // don't shrink C.
----------------
For IR, the target effects are a secondary concern. We should just say something like:
"We expect this is better for codegen because it allows forming smaller immediate values."
The primary motivation is that it makes IR analysis easier and may help with SCEV.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:57
+  // target-independent and should NOT use TTI.
+  // Note(2): The rationale for adding this check here and not just undoinging
+  // this in the backend is that llvm.assume can inform necessary bits, but is
----------------
undoinging -> undoing


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:59
+  // this in the backend is that llvm.assume can inform necessary bits, but is
+  // not propegated to the backend, so there is not always enough information to
+  // undo this transformation.
----------------
propegated -> propagated


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:64-65
+    unsigned BitReqAfter = PowerOf2Ceil(ShrunkC.getSignificantBits());
+    // Will take more sign-extended bits after and greater than 1-byte (assumed
+    // minimum type size).
+    if (BitReqAfter > BitReqBefore && BitReqAfter > 8)
----------------
I'd change this sentence to make the outcome we're looking for explicit:
  // For anything larger than an 8-bit (byte) constant, avoid changing a 
  // small magnitude negative number into a larger magnitude number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140858