[PATCH] D108049: [InstCombine] Extend canonicalizeClampLike to handle truncated inputs

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1427
+  Value *MaybeReplacedLow = Builder.CreateSelect(
+      ShouldReplaceLow, Builder.CreateSExt(ReplacementLow, X->getType()), X);
+
----------------
lebedev.ri wrote:
> We didn't need `sext` originally. Why do we always need one now? Should this use `CreateSExtOrBitCast()`?
It is relying on the CreateSExt doing `if (V->getType() == DestTy) return V;`. I can change that to CreateSExtOrBitCast but It shouldn't ever bitcast anything, the types will always be integers.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1431-1438
+  if (X->getType() == Sel0.getType())
+    return SelectInst::Create(ShouldReplaceHigh, ReplacementHigh,
+                              MaybeReplacedLow);
+
+  Value *MaybeReplacedHigh = Builder.CreateSelect(
+      ShouldReplaceHigh, Builder.CreateSExt(ReplacementHigh, X->getType()),
+      MaybeReplacedLow);
----------------
lebedev.ri wrote:
> I would recommend something along the lines of using `CreateSExtOrBitCast()`,
> only calling `CreateSelect` in a single place, and returning through `CreateTruncOrBitCast()`.
Oh yeah I was awkwardly working around it returning an intruction. I've changed it, let be know if it looks wrong or they should be changed to `OrBitcast` versions.


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

https://reviews.llvm.org/D108049