[PATCH] D54653: Correct CreateAlignmentAssumption to check sign and power-of-2 (LLVM Part)
Erich Keane via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 26 06:59:14 PST 2018
erichkeane marked an inline comment as done.
erichkeane added inline comments.
================
Comment at: include/llvm/IR/IRBuilder.h:2201-2203
+ Value *IsPositive = CreateICmp(
+ Sign == AlignmentSign::Signed ? CmpInst::ICMP_SGT : CmpInst::ICMP_UGT,
+ Alignment, ConstantInt::get(Alignment->getType(), 0), "ispositive");
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > erichkeane wrote:
> > > lebedev.ri wrote:
> > > > Would it be better to only keep the predicate change, and leave the power-of-two as UB?
> > > We already have that before this change, though it has a mis-signedness. However, there isn't really issue treating it as signed always, since values outside of the range of a signed value are illegal/UB anyway. IMO, this would be the 'don't change things' opinion.
> > Replied to the wrong comment maybe?
> > The question was - do we really want to do `@llvm.assume()` if the requested alignment
> > is really power-of-two at the run time? That will somewhat confuse the middle-end,
> > and is UB in any case, as @hfinkel said.
> err
>
> The question was - do we really want to ONLY do `@llvm.assume()` if the requested alignment
> is REALLY power-of-two at the run time? That will somewhat confuse the middle-end,
> and is UB in any case, as @hfinkel said.
Perhaps I'm missing something (it IS monday :) ), but I would presume that the cost of a branch/more BBs to determine that (rather than a select) would be more harmful than just doing a no-op assume.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54653/new/
https://reviews.llvm.org/D54653
More information about the llvm-commits
mailing list