[PATCH] D33186: [InstCombine] Canonicalize clamp of float types to minmax in fast mode.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1326
+      bool IsCastNeeded = LHS->getType()->getPrimitiveSizeInBits() !=
+                              SelType->getPrimitiveSizeInBits();
+      Value *CmpLHS = cast<CmpInst>(CondVal)->getOperand(0);
----------------
a.elovikov wrote:
> a.elovikov wrote:
> > efriedma wrote:
> > > This is very suspicious.
> > > 
> > > The way the implementation of matchSelectPattern is written, CastOp is uninitialized if the type of the compare matches the type of the select; otherwise, it's set to whatever cast we looked through.  That cast might not be a cast which changes the size of the type; it could bit a BitCast/FPToUI/etc.
> > > 
> > > I'd like to see a few testcases which cover the situations where we insert casts.
> > Yes, this exactly the case causing UB - passing uninitialized  object by value to createCast Corresponding argument is not even used in that function because of the early return when the types are equal.
> > 
> > I believe the case with cast should be already covered by earlier tests as I did not touch that part - will try to find them tomorrow, or add new ones if they're missing.
> One more note - neither Aslan nor MSan catches this, only UBSan because the value of the uninitialized argument to createCast is not used.
> I believe the case with cast should be already covered by earlier tests as I did not touch that part - will try to find them tomorrow, or add new ones if they're missing.

I'm specifically concerned about cases where transforming a floating-point clamp requires inserting a cast.


https://reviews.llvm.org/D33186