[TableGen] Do not set ReadOnly attribute on intrinsics with side effects

[TOCK]

Summary

If an intrinsic with side effects is called twice using the same set of
arguments, EarlyCSE would happily remove the second call because it has
ReadOnly attribute. Similar to 52c39396151978ca946e2a80d9118c8672bace14,
this patch makes TableGen not set Attribute::ReadOnly for intrinsics
which are declared with IntrHasSideEffects.

[TOCK]

For example, say we have these intrinsics:

def int_test_no_mem_v : Intrinsic<[], [llvm_i64_ty], [IntrNoMem, IntrHasSideEffects]>;
def int_test_read_mem_v : Intrinsic<[], [llvm_i64_ty], [IntrReadMem, IntrHasSideEffects]>;
def int_test_no_mem : Intrinsic<[llvm_i64_ty], [llvm_i64_ty], [IntrNoMem, IntrHasSideEffects]>;
def int_test_read_mem : Intrinsic<[llvm_i64_ty], [llvm_i64_ty], [IntrReadMem, IntrHasSideEffects]>;

Following code:

define i64 @foo(i64 %a, i64 %b) {
entry:
  call void @llvm.test.no.mem.v(i64 %a)
  call void @llvm.test.no.mem.v(i64 %a)
  call void @llvm.test.read.mem.v(i64 %b)
  call void @llvm.test.read.mem.v(i64 %b)
  %r1 = call i64 @llvm.test.no.mem(i64 %a)
  %r3 = call i64 @llvm.test.no.mem(i64 %a)
  %r2 = call i64 @llvm.test.read.mem(i64 %b)
  %r4 = call i64 @llvm.test.read.mem(i64 %b)
  %r12 = add nsw i64 %r1, %r2
  %r34 = add nsw i64 %r3, %r4
  %r14 = add nsw i64 %r12, %r34
  ret i64 %r14
}
declare void @llvm.test.no.mem.v(i64)
declare void @llvm.test.read.mem.v(i64)
declare i64 @llvm.test.no.mem(i64)
declare i64 @llvm.test.read.mem(i64)

would be optimized into:

define i64 @foo(i64 %a, i64 %b) local_unnamed_addr #0 {
entry:
  tail call void @llvm.test.no.mem.v(i64 %a)
  tail call void @llvm.test.no.mem.v(i64 %a)
  %r1 = tail call i64 @llvm.test.no.mem(i64 %a)
  %r3 = tail call i64 @llvm.test.no.mem(i64 %a)
  %r2 = tail call i64 @llvm.test.read.mem(i64 %b)
  %factor = shl i64 %r2, 1
  %r12 = add i64 %r3, %r1
  %r14 = add i64 %r12, %factor
  ret i64 %r14
}
...

Calls to @llvm.test.read.mem.v are removed completely (this is what D64414 tried to address), also one call to @llvm.test.read.mem is removed:

EarlyCSE DCE:   call void @llvm.test.read.mem.v(i64 %b)                                                                                           
EarlyCSE DCE:   call void @llvm.test.read.mem.v(i64 %b)                                                                                           
EarlyCSE CSE CALL:   %r4 = call i64 @llvm.test.read.mem(i64 %b)  to:   %r2 = call i64 @llvm.test.read.mem(i64 %b)                                 
        discovered a new reachable node %entry

This is due to the fact that TableGen doesn't respect IntrHasSideEffects, and make it read only:

...
    case 52: {
      const Attribute::AttrKind Atts[] = {Attribute::NoUnwind,Attribute::ReadOnly};
      AS[0] = AttributeList::get(C, AttributeList::FunctionIndex, Atts);
      NumAttrs = 1;
      break;
      }
...

[danilaml]

The diffs lack context.

It looks like there is some inconsistency between .td attributes/properties and IR ones.
As far as I can tell there is no way in IR to mark something as "this doesn't touch mem but may have other side-effects" so it'd be safe to, for example, move some load past that instruction but not to DCE/CSE it.

[TOCK]

Hi @danilaml,

The diffs lack context.

This patch is some kind of follow-up of D64414, which make TableGen stop emitting Attribute::ReadNone for IntrNoMem if such intrinsic is marked as IntrHasSideEffects. This patch does the same for other cases like IntrReadMem.

As far as I can tell there is no way in IR to mark something as "this doesn't touch mem but may have other side-effects" so it'd be safe to, for example, move some load past that instruction but not to DCE/CSE it.

As far I can tell most targets simply mark it with side-effects and rely on the backend to do optimization, since it might be able to model the side-effects and have a clearer view of what is really accessed.

[danilaml]

@TOCK
I've meant the surrounding context for the diff (https://llvm.org/docs/Phabricator.html#phabricator-request-review-web),
i.e. git show HEAD -U999999

As far I can tell most targets simply mark it with side-effects and rely on the backend to do optimization, since it might be able to model the side-effects and have a clearer view of what is really accessed.

The problem is that is that it looks like the info is lost.
It is also unclear what is the effect of IntrNoMem/IntrReadMem, IntrHasSideEffects now. From documentation/patch it seems the original intent was to mark intrinsics that don't modify/use memory (so optimizations that rely on mayWriteMemory will work for them), but have some non memory related side-effects (so the wouldn't be CSE's/DCE'd and etc.).
But after this and D64414 patches such intrinsics would return mayWriteMemory, which feels counter-intuitive and against the initial intent. I don't know how to properly fix this (and at what level this problem lies).

[lebedev.ri]

Which LLVM IR intrinsics are affected by this?

[TOCK]

Include context as @danilaml suggested.

[TOCK]

@danilaml
+1. It looks like a workaround at the moment.

@lebedev.ri
I'm not aware of any in-tree intrinsic affected. The affected one I found is generated by some testing tool.

[craig.topper]

In D76127#2421901, @TOCK wrote:

@danilaml
+1. It looks like a workaround at the moment.

@lebedev.ri
I'm not aware of any in-tree intrinsic affected. The affected one I found is generated by some testing tool.

Probably no in-tree intrinsic is affected because the current code causes SelectionDAGBuilder to generate broken code. I believe this is the FIXME on int_x86_sse_ldmxcsr in llvm/include/IR/IntrinsicsX86.td

[craig.topper]

Having just tripped over this bug again. I think we should fix this

LGTM

[lebedev.ri]

I'd like for @jdoerfert to comment.
Can we simply disallow this somewhere and catch such a combination in a verifier or something?

[jdoerfert]

TBH, I feel "X is readonly and has side effects" sends the wrong message to begin with. It is a contradiction (in the IR world) as basically shown by the need for this patch. Given that there are no examples in-tree I don't understand why one would mark a side-effect intrinsic as readonly (or similar). Long story short, I would argue this should be a loud error, not silently ignored.

[craig.topper]

In D76127#2510504, @jdoerfert wrote:

TBH, I feel "X is readonly and has side effects" sends the wrong message to begin with. It is a contradiction (in the IR world) as basically shown by the need for this patch. Given that there are no examples in-tree I don't understand why one would mark a side-effect intrinsic as readonly (or similar). Long story short, I would argue this should be a loud error, not silently ignored.

Why shouldn't the intrinsics file be able to express the semantics we want even if we can't represent it in IR today?

The annoying issue is that tablegen also uses the WriteMem to force the mayStore flag on any machine IR instruction that has a pattern that references the intrinsic. Machine IR has a separate side effects flag. So we want IntrReadMem+IntrHasSideEffects to only set mayLoad+hasSideEffects in machine IR.

And there are no examples in tree because it breaks today. See the FIXME on ldmxcsr. It causes SelectionDAGBuilder to fail to connect the output chain to the root node in the DAG which makes the intrinsic eligible for deletion when it shouldn't be.

[craig.topper]

llvm/utils/TableGen/IntrinsicEmitter.cpp

836

Thinking about this again, I think the entire switch should be skipped if hasSideEffects is set. None of the cases are valid.

Which is implied by the if statement earlier checking

(intrinsic.ModRef != CodeGenIntrinsic::ReadWriteMem && !intrinsic.hasSideEffects)

[jdoerfert]

In D76127#2510562, @craig.topper wrote:

In D76127#2510504, @jdoerfert wrote:

TBH, I feel "X is readonly and has side effects" sends the wrong message to begin with. It is a contradiction (in the IR world) as basically shown by the need for this patch. Given that there are no examples in-tree I don't understand why one would mark a side-effect intrinsic as readonly (or similar). Long story short, I would argue this should be a loud error, not silently ignored.

Why shouldn't the intrinsics file be able to express the semantics we want even if we can't represent it in IR today?

My position hasn't changed much from before: https://reviews.llvm.org/D64414#1584449

I mean, the way we interpret the bits for the IR is contradictory, isn't it? In IR, you cannot have an intrinsic that is readonly and has side-effect. Why would we want to express that in the intrinsics file if it is impossible and probably a sign of a misconfiguration. Maybe I misunderstand what "side-effects" are supposed to be here but I find it confusing to say "readonly with side-effects". Interestingly, this "handling" of such a configuration disables middle-end optimizations to enable backend optimizations, which may or may not be worth it.

FWIW, I believe adding more categories other than "inaccessible memory" and "argument memory" is the right way to resolve this issue. Also, side-effect free instructions that are not willreturn should not be deleted in the future. This might not be interesting for the ldmxcsr case (which I don't know what the side-effect is), but for D64414 this might be the proper
way to model it.

[craig.topper]

Revoking my approval

[arsenm]

I also don't think readonly makes sense with side effects

llvm/test/TableGen/intrin-side-effects.td

   int isAny = 0;
 }
 
+class LLVMQualPointerType<LLVMType elty, int addrspace>
+  : LLVMType<iPTR>{
+  LLVMType ElTy = elty;
+  int AddrSpace = addrspace;
+}
+
+class LLVMPointerType<LLVMType elty>
+  : LLVMQualPointerType<elty, 0>;
+
+def llvm_i8_ty         : LLVMType<i8>;
 def llvm_i32_ty        : LLVMType<i32>;
+def llvm_ptr_ty        : LLVMPointerType<llvm_i8_ty>;
 
 class IntrinsicProperty;
 def IntrNoMem : IntrinsicProperty;
 def IntrHasSideEffects : IntrinsicProperty;
+def IntrReadMem : IntrinsicProperty;
+def IntrArgMemOnly : IntrinsicProperty;
+def IntrInaccessibleMemOnly : IntrinsicProperty;
+def IntrInaccessibleMemOrArgMemOnly : IntrinsicProperty;
 
 
 class Intrinsic<list<LLVMType> ret_types,

   bit isTarget = 0;
 }
 
-// ... this intrinsic.
-def int_random_gen   : Intrinsic<[llvm_i32_ty], [], [IntrNoMem, IntrHasSideEffects]>;
+// ... these intrinsic.
+
+// CHECK: 1, // llvm.random.gen.no.mem
+// CHECK: 2, // llvm.random.gen.read.arg.mem
+// CHECK: 3, // llvm.random.gen.read.inaccessible.mem
+// CHECK: 4, // llvm.random.gen.read.inaccessible.mem.or.arg.mem
+// CHECK: 5, // llvm.random.gen.read.mem
+
+// CHECK: case 5:
+// CHECK-NEXT: Atts[] = {Attribute::NoUnwind}
+def int_random_gen_read_mem
+    : Intrinsic<[llvm_i32_ty], [], [IntrReadMem, IntrHasSideEffects]>;
+
+// CHECK: case 4:
+// CHECK-NEXT: Atts[] = {Attribute::NoUnwind}
+def int_random_gen_read_inaccessible_mem_or_arg_mem
+    : Intrinsic<[llvm_i32_ty], [], [IntrReadMem,IntrInaccessibleMemOrArgMemOnly,
+                                    IntrHasSideEffects]>;
+
+// CHECK: case 2:
+// CHECK-NEXT: Atts[] = {Attribute::NoUnwind}
+def int_random_gen_read_arg_mem
+    : Intrinsic<[llvm_i32_ty], [llvm_ptr_ty], [IntrReadMem, IntrArgMemOnly,
+                                               IntrHasSideEffects]>;
+
+// CHECK: case 3:
+// CHECK-NEXT: Atts[] = {Attribute::NoUnwind}
+def int_random_gen_read_inaccessible_mem
+    : Intrinsic<[llvm_i32_ty], [], [IntrReadMem, IntrInaccessibleMemOnly,
+                                    IntrHasSideEffects]>;
 
-// CHECK: 1, // llvm.random.gen
 // CHECK: case 1:
 // CHECK-NEXT: Atts[] = {Attribute::NoUnwind}
+def int_random_gen_no_mem
+    : Intrinsic<[llvm_i32_ty], [], [IntrNoMem, IntrHasSideEffects]>;

llvm/utils/TableGen/IntrinsicEmitter.cpp

         OS << "Attribute::ReadNone";
         break;
       case CodeGenIntrinsic::ReadArgMem:
+        if (intrinsic.hasSideEffects)
+          break;
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadOnly,";
         OS << "Attribute::ArgMemOnly";
         break;
       case CodeGenIntrinsic::ReadMem:
+        if (intrinsic.hasSideEffects)
+          break;
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadOnly";
         break;
       case CodeGenIntrinsic::ReadInaccessibleMem:
+        if (intrinsic.hasSideEffects)
+          break;
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadOnly,";
         OS << "Attribute::InaccessibleMemOnly";
         break;
       case CodeGenIntrinsic::ReadInaccessibleMemOrArgMem:
+        if (intrinsic.hasSideEffects)
+          break;
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadOnly,";