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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I guess alignment is one of the main users of the old format, great to see it go :)

First set of comments.



================
Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:104
+    return 1;
+  };
   if (BOI.End - BOI.Begin > ABA_Argument)
----------------
I think this is a problem for things like `deref` which should default to 0?


================
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;
----------------
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?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4196
       LHS->setMetadata(LLVMContext::MD_nonnull, MD);
-      return eraseInstFromFunction(*II);
+      if (!HasOpBundles)
+        return eraseInstFromFunction(*II);
----------------
Can we split these changes off. Seem unrelated and easier.


================
Comment at: llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp:273
+      if (!isValidAssumeForContext(ACall, J, DT))
+        continue;
       Align NewDestAlignment =
----------------
I'm curious, why was this needed? 


================
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
----------------
Can you add the "beginning" of the operand bundles here so it becomes clear we do not have a no-op assume.


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

https://reviews.llvm.org/D71739