[PATCH] D71739: [AssumeBundles] Use operand bundles to encode alignment assumptions

[Tyker via Phabricator via cfe-commits]

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