[PATCH] LLVM intrinsic for invariants

Philip Reames listmail at philipreames.com
Mon Jul 21 10:04:45 PDT 2014


With a couple of small changes mentioned inline, LGTM.

================
Comment at: docs/LangRef.rst:9383
@@ +9382,3 @@
+mathematical invariants that the optimizer can otherwise deduce or facts that
+are of little use to the optimizer.
+
----------------
Why?  Suggestion:
Please note that ''llvm.assume'' is not free and may impact the effectiveness of the optimizer in hard to predict ways.  ''llvm.assume'' should be used to represent high level facts which are not otherwise available to the optimizer and only after careful consideration of the benefit and costs.  In particular, 'llvm.assume'' should not be used to...


I'm not sure I really like my wording around 'free' above, but this is a starting point.  

================
Comment at: include/llvm/IR/Intrinsics.td:282
@@ +281,3 @@
+// control dependencies will be maintained.
+def int_assume        : Intrinsic<[], [llvm_i1_ty], []>;
+
----------------
I understand why you're doing this, but it honestly feels like a bit of a hack.  Is it possibly time to introduce a notion of control dependence that is not driven by memory access?  This is essentially what you've written, but there might be a better way to factor it.  

This doesn't need to be changed before submission, I'm just raising it for consideration and discussion.  

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:799
@@ -801,1 +798,3 @@
 
+static bool isAssumeIntrinsic(ImmutableCallSite CS) {
+  const IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction());
----------------
I've noticed we have multiple isXIntrinsic functions scatter around the code base.  We might want to think about - in a separate change - creating an isIntrinsic(IntrinsicID) function on CallSite.  

================
Comment at: lib/Transforms/Utils/Local.cpp:1199
@@ +1198,3 @@
+          if (isa<UndefValue>(II->getArgOperand(0)))
+            MakeUnreachable = true;
+          else if (ConstantInt *Cond =
----------------
Adding a comment on this case would be good.  Particularly given that it required discussion to arrive at this being the right semantic.  

================
Comment at: test/CodeGen/Generic/assume.ll:1
@@ +1,2 @@
+; RUN: llc < %s
+
----------------
This test could be stronger.  Could you add a FileCheck here?

http://reviews.llvm.org/D178






More information about the llvm-commits mailing list