Tyker added a comment.
In D71739#2048409 <https://reviews.llvm.org/D71739#2048409>, @rjmccall wrote:
> Is there a good reason for this to use the same `llvm.assume` intrinsic as
> before?
thee hasn't been any discussion i am aware of on this topic.
but personally i think keeping the same intrinsic makes sens because it
logically expresses an assumption hence `assume` is a good name.
and most passes don't need to treat classic assumes differently from assume
with operand bundles.
> Are there restrictions about what assumptions can be combined on a single
> intrinsic call? There can only be one bundle of a particular name on an
> instruction, right?
there is any IR legality restriction about what combination or number of
bundles can be present in an assume. however there is restrictions about
argument of a given bundle.
having a single "align" bundle is just how the front-end used assume bundles
for its alignment assumptions.
for example:
to salvage a store like
`store i8 0, i8* %P, align 1`
we could generate
`call void @llvm.assume(i1 true) [ "dereferenceable"(i8* %P, i64 1),
"nonnull"(i8* %P) ]`
================
Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:104
+ return 1;
+ };
if (BOI.End - BOI.Begin > ABA_Argument)
----------------
jdoerfert wrote:
> I think this is a problem for things like `deref` which should default to 0?
currently only Alignment can have a non constant argument. but yes this will
change if we support non-constant integer for deref.
the verifier has been updated this way, but i added an assert to make it clear.
================
Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:110
+ if (BOI.End - BOI.Begin > ABA_Argument + 1)
+ Result.ArgValue = MinAlign(Result.ArgValue, GetArgOr1(1));
return Result;
----------------
jdoerfert wrote:
> Is this the min of the alignment and offset? If so, I'm not sure about min.
> Either way, can you clang format this and add a comment why alignment is
> special and how it looks?
> Is this the min of the alignment and offset?
Yes
> If so, I'm not sure about min
the objective of this is to make sure that getKnowledgeFromBundle doesn't
misinterpret the contents of a bundles with a non-zero offset.
the description of MinAlign seems to match with what i needed
```
/// A and B are either alignments or offsets. Return the minimum alignment that
/// may be assumed after adding the two together.
```
the results seems to be correct aswell.
================
Comment at: llvm/lib/IR/Verifier.cpp:4418
+ if (ArgCount == 3)
+ Assert(Call.getOperand(Elem.Begin + 2)->getType()->isIntegerTy(),
"third argument should be an integer if present");
+ return;
----------------
rjmccall wrote:
> Should the alignment and offset be restricted to constants and given value
> restrictions?
the system operand bundles are replacing could deal with non-constant indexes
and offsets
so i adapted alignment assume bundles to handle non-constant arguments.
================
Comment at: llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp:273
+ if (!isValidAssumeForContext(ACall, J, DT))
+ continue;
Align NewDestAlignment =
----------------
jdoerfert wrote:
> I'm curious, why was this needed?
this code is part of the from the previous version of the patch, i left it
untouched because it seemed to work as intended.
changes in this function seem to have no impact on the resulting behavior. and
could be removed.
================
Comment at: llvm/test/Transforms/InstCombine/assume.ll:380
; CHECK-NEXT: [[CMP2:%.*]] = icmp ne i8 [[X:%.*]], 0
+; CHECK-NEXT: tail call void @llvm.assume(i1 false)
; CHECK-NEXT: tail call void @llvm.dbg.value(metadata i32 5, metadata !7,
metadata !DIExpression()), !dbg !9
----------------
jdoerfert wrote:
> Can you add the "beginning" of the operand bundles here so it becomes clear
> we do not have a no-op assume.
i don't think `call void @llvm.assume(i1 false)` is no-op since it exhibits UB.
but it is redundant.
anyway this has been split of to an other patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71739/new/
https://reviews.llvm.org/D71739
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
