Page MenuHomePhabricator

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

Authored by TOCK on Mar 13 2020, 5:26 AM.

Details

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.

Diff Detail

Event Timeline

TOCK created this revision.Mar 13 2020, 5:26 AM
TOCK created this object with visibility "All Users".
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2020, 5:26 AM
TOCK added a comment.Mar 15 2020, 8:32 PM

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;
      }
...
TOCK added a reviewer: arsenm.Sep 28 2020, 3:38 AM
TOCK changed the visibility from "All Users" to "Public (No Login Required)".

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 added a comment.Nov 26 2020, 6:12 PM

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.

@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).

Which LLVM IR intrinsics are affected by this?

TOCK updated this revision to Diff 308253.Nov 29 2020, 7:12 PM

Include context as @danilaml suggested.

TOCK added a comment.Nov 29 2020, 7:23 PM

@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.

@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