[PATCH] D82703: [InstCombine] convert assumes to operand bundles

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:114
+    return ArgValue < Other.ArgValue;
+  }
   operator bool() const { return AttrKind != Attribute::None; }
----------------
Nit: Add a message to the assert please explaining what usage is allowed.


================
Comment at: llvm/lib/Analysis/Loads.cpp:94
+      DerefRK.ArgValue >= Size.getZExtValue())
+    return true;
+
----------------
Nit: Add a comment describe what is happening here.

I guess we should have a todo that states partial information, e.g., only AlignRK or DerefRK, should be used in other places too. The entire function should be rewritten for that so it is out of scope.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4299
+          /// Note: this doesn't preserve the offset information but merges
+          /// offset and alignememt.
+          RetainedKnowledge RK{Attribute::Alignment,
----------------
jdoerfert wrote:
> Shouldn't we build a GEP for the offset instead, if the offset is not a multiple of the alignment that is.
At least a TODO would be good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82703