Page MenuHomePhabricator

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

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

craig.topper accepted this revision.Jan 20 2021, 11:27 AM

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

LGTM

This revision is now accepted and ready to land.Jan 20 2021, 11:27 AM

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

jdoerfert added a comment.EditedJan 20 2021, 11:36 AM

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 added a comment.EditedJan 20 2021, 11:55 AM

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 added inline comments.Jan 20 2021, 12:03 PM
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)

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 requested changes to this revision.Jan 20 2021, 3:42 PM

Revoking my approval

This revision now requires changes to proceed.Jan 20 2021, 3:42 PM