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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits