[PATCH] D65380: [InstCombine] Shift amount reassociation: shl-trunc-shl pattern

[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



================
Comment at: llvm/lib/IR/Constants.cpp:1590-1594
+Constant *ConstantExpr::getZExtOrSelf(Constant *C, Type *Ty) {
+  if (C->getType() == Ty)
+    return C;
+  return getZExt(C, Ty);
+}
----------------
lebedev.ri wrote:
> spatel wrote:
> > Would we better off copying the codegen sibling API (same type implicitly returns self)?
> > 
> > SelectionDAG::getZExtOrTrunc()
> I'm not sure what you have in mind here, `ConstantExpr::getZExt()`
> will (rightfully) assert if zext is unsuitable there.
> `ConstantExpr::getBitCast()` won't, so i suppose i could just use it,
> but i really don't need bitcast here, i really want either zext or nothing.
> 
> Please can you be more specific as to what you envision here?
I'm imagining that we'll eventually (or might already) want the flexibility of returning a constant of arbitrary type (wider or narrower) since that's used in codegen regularly. But that can be a follow-on if we want.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:39
+
+  // There may be a truncation before the next shift; take note and ignore it.
   Instruction *Sh1;
----------------
lebedev.ri wrote:
> spatel wrote:
> > That comment isn't clear to me.
> >   "If there is truncation of the shifted value, we must 
> >   adjust the constraints of this fold and recreate that op." ?
> How about:
> ```
> If there is a truncation inbetween these two shifts,
> we must make note of it, and look through it.
> The truncation will impose additional legality checks
> on the transform.
> ```
> ?
That works...just some minor adjustments:
  If there is a truncation between the two shifts,
  we must make note of it and look through it.
  The truncation imposes additional constraints
  on the transform.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65380