[PATCH] D67799: [InstCombine] Fold a shifty implementation of clamp0.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

We need to confirm that the backend produces better asm for at least a few in-tree targets before/after this transform. Please attach output for x86 and AArch64. We'll want to have examples for scalar and vector code, so you probably need to suppress the vectorizers.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1940
+          Builder.CreateICmpSLT(V, ConstantInt::getNullValue(Ty));
+      return SelectInst::Create(NewICmpInst, ConstantInt::getNullValue(Ty), V);
+    }
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > Hmm, super random thought.
> > @spatel we convert code that was written without a branch, likely very intentionally,
> > into a possibly-branch code. Should we not add `unpredictable` to this new `switch`?
> > I think it's almost correctness question..
> s/switch/select/
It's not a matter of correctness, but I agree that we do not want to end up with branchy code when the source used tricky bit-hacks almost certainly to avoid branching. 

But adding 'unpredictable' is not a solution here AFAICT because we're not creating a branch or switch. We could add explicit profile metadata to the select to indicate the compare is 50/50, but that doesn't necessarily imply unpredictable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67799