Do not set ReadNone attribute on intrinsics with side effects

[chill]

Summary

If an intrinsic is defined without outputs, but having side effects, it still can be removed completely from
the program. This patch make TableGen not set Attribute::ReadNone for intrinsics which
are declared with IntrHasSideEffects.

[lebedev.ri]

Test?

[chill]

In D64414#1575868, @lebedev.ri wrote:

Test?

Test is the __builtin_arm_tcommit two steps up in the stack, but I guess I can add a dedicated one.

[lebedev.ri]

If an intrinsic is defined without outputs, but having side effects, it still can be removed completely from the program

This kinda sounds slightly backwards.
*If* there are side-effects, then i'd expect it not to be dropped regardless of readnone status.

[chill]

In D64414#1575894, @lebedev.ri wrote:

If an intrinsic is defined without outputs, but having side effects, it still can be removed completely from the program

This kinda sounds slightly backwards.
*If* there are side-effects, then i'd expect it not to be dropped regardless of readnone status.

Fair enough.

I had the issue that the tcommit (see child patch) was being discarded during instruction selection because

static bool isFoldedOrDeadInstruction(const Instruction *I,
                                      FunctionLoweringInfo *FuncInfo) {
  return !I->mayWriteToMemory() && // Side-effecting instructions aren't folded.
         !I->isTerminator() &&     // Terminators aren't folded.
         !isa<DbgInfoIntrinsic>(I) &&  // Debug instructions aren't folded.
         !I->isEHPad() &&              // EH pad instructions aren't folded.
         !FuncInfo->isExportedInst(I); // Exported instrs must be computed.
}

was true for the call insn.

Is the better fix adding a function attribute, e.g def SideEffects : EnumAttr<"sideeffects">; , etc and then changing the expression to be

  return !I->mayWriteToMemory() && // Side-effecting instructions aren't folded.
               !I->hasSideEffects() && // Has unspecified side effects
               !I->isTerminator() &&     // Terminators aren't folded.
....

[jdoerfert]

I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?

[chill]

In D64414#1576252, @jdoerfert wrote:

I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?

E.g __builtin_arm_tcommit, which I'm adding in this series, or __builtin_arm_irg (which is *should* be NoMem, but is not, perhaps as a workaround for this issue).

[jdoerfert]

In D64414#1576554, @chill wrote:

In D64414#1576252, @jdoerfert wrote:

I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?

E.g __builtin_arm_tcommit, which I'm adding in this series, or __builtin_arm_irg (which is *should* be NoMem, but is not, perhaps as a workaround for this issue).

(Sorry if I'm way off here but I'd like to understand this better)

tcommit makes the transaction state visible, correct? If so, I'd argue it modifies the memory (especially for other threads). Maybe my confusion stems from the IR vs MIR conflict here but in IR, I would not like tcommit to be readnone for multiple reasons, including memory movement around it.

[chill]

In D64414#1576588, @jdoerfert wrote:

In D64414#1576554, @chill wrote:

In D64414#1576252, @jdoerfert wrote:

I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?

E.g __builtin_arm_tcommit, which I'm adding in this series, or __builtin_arm_irg (which is *should* be NoMem, but is not, perhaps as a workaround for this issue).

(Sorry if I'm way off here but I'd like to understand this better)

tcommit makes the transaction state visible, correct? If so, I'd argue it modifies the memory (especially for other threads). Maybe my confusion stems from the IR vs MIR conflict here but in IR, I would not like tcommit to be readnone for multiple reasons, including memory movement around it.

tcommit does not modify memory (at least in my understanding what "modify memory" means), in the sense that if I do:

*p = x;
tcommit();
z = *p

I would expect x == z, i.e the above to be equivalent to

*p = x;
tcommit();
z = x;

Definitely we wouldn't like memory operations to be moved around transactional primitives.

[jdoerfert]

In D64414#1576859, @chill wrote:

In D64414#1576588, @jdoerfert wrote:

In D64414#1576554, @chill wrote:

In D64414#1576252, @jdoerfert wrote:

I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?

E.g __builtin_arm_tcommit, which I'm adding in this series, or __builtin_arm_irg (which is *should* be NoMem, but is not, perhaps as a workaround for this issue).

(Sorry if I'm way off here but I'd like to understand this better)

tcommit makes the transaction state visible, correct? If so, I'd argue it modifies the memory (especially for other threads). Maybe my confusion stems from the IR vs MIR conflict here but in IR, I would not like tcommit to be readnone for multiple reasons, including memory movement around it.

tcommit does not modify memory (at least in my understanding what "modify memory" means), in the sense that if I do:

*p = x;
tcommit();
z = *p

I would expect x == z, i.e the above to be equivalent to

*p = x;
tcommit();
z = x;

Definitely we wouldn't like memory operations to be moved around transactional primitives.

What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.

[chill]

In D64414#1576955, @jdoerfert wrote:

What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.

If the transaction is aborted before or by the tcommit, it'll be automatically rolled back to the tstart, which initiated it, discarding
any register and memory writes, and won't reach the instruction after the tcommit.

Anyway, I think we are drifting towards discussion of the correct way to implement transactional memory and its interactions with the memory model, which
is a very valuable discussion, that I suggest we continue here: https://reviews.llvm.org/D64416

As for the function attributes, etc, maybe a simpler use case would be this one: https://reviews.llvm.org/D64447 (A little bit of background, I suggested that might be a correct change in a comment here: https://reviews.llvm.org/D60486#inline-574695)

[jdoerfert]

In D64414#1577844, @chill wrote:

In D64414#1576955, @jdoerfert wrote:

What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.

If the transaction is aborted before or by the tcommit, it'll be automatically rolled back to the tstart, which initiated it, discarding
any register and memory writes, and won't reach the instruction after the tcommit.

I agree, that is what needs to happen. Maybe I misunderstood what your example was supposed to show. The way I did understand it, I figured you want the store to load propagation to happen across the tcommit instruction. That has to be forbidden in the IR as it will otherwise happen and break the atomicity of the transaction. To forbid it one would give the tcommit intrinsic a memory effect. Now, I think it actually has a memory effect: Similar to sequential consistent atomic loads, tcommit synchronizes, thereby making memory changes visible to the thread.

We can move the discussion to the main patch if you want.

[chill]

In D64414#1578661, @jdoerfert wrote:

In D64414#1577844, @chill wrote:

In D64414#1576955, @jdoerfert wrote:

What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.

If the transaction is aborted before or by the tcommit, it'll be automatically rolled back to the tstart, which initiated it, discarding
any register and memory writes, and won't reach the instruction after the tcommit.

I agree, that is what needs to happen. Maybe I misunderstood what your example was supposed to show. The way I did understand it, I figured you want the store to load propagation to happen across the tcommit instruction. That has to be forbidden in the IR as it will otherwise happen and break the atomicity of the transaction. To forbid it one would give the tcommit intrinsic a memory effect. Now, I think it actually has a memory effect: Similar to sequential consistent atomic loads, tcommit synchronizes, thereby making memory changes visible to the thread.

We can move the discussion to the main patch if you want.

Sorry, I wasn't clear, my example was just to clarify what I mean by "does not modify memory".

So, back the original motivation for the patch, let's look at __builtin_arm_ttest and __builtin_arm_irg (which map 1:1 to an LLVM intrnisic and to an AArch64 instruction, so I use a short name to refer to all three)

• ttest is set to have side affects, so it's not reordered with tstart and tcommit; it's OK to move memory reads/writes around it, so it's also IntrNoMem; in that state tablegen would

set readnone attribute. Bu then for readnone:

On a function, this attribute indicates that the function computes its result (or decides to unwind an exception) based strictly on its arguments, without dereferencing any pointer arguments or otherwise accessing any mutable state (e.g. memory, control registers, etc)

and ttest *does* access mutable state. TBH, if the result of ttest is dead, there's no problem discarding the call, it just contradicts with the LLVM spec.

• It's also fine to reorder memory read/writes around irg; but it has actual side-effects, so it's not OK to discard it even if its result is read; if we define irg with IntrNoMem and IntrHasSideEffect it'll get readnone and might be discarded.

[jdoerfert]

In D64414#1580538, @chill wrote:

In D64414#1578661, @jdoerfert wrote:

In D64414#1577844, @chill wrote:

In D64414#1576955, @jdoerfert wrote:

What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.

If the transaction is aborted before or by the tcommit, it'll be automatically rolled back to the tstart, which initiated it, discarding
any register and memory writes, and won't reach the instruction after the tcommit.

I agree, that is what needs to happen. Maybe I misunderstood what your example was supposed to show. The way I did understand it, I figured you want the store to load propagation to happen across the tcommit instruction. That has to be forbidden in the IR as it will otherwise happen and break the atomicity of the transaction. To forbid it one would give the tcommit intrinsic a memory effect. Now, I think it actually has a memory effect: Similar to sequential consistent atomic loads, tcommit synchronizes, thereby making memory changes visible to the thread.

We can move the discussion to the main patch if you want.

Sorry, I wasn't clear, my example was just to clarify what I mean by "does not modify memory".

So, back the original motivation for the patch, let's look at __builtin_arm_ttest and __builtin_arm_irg (which map 1:1 to an LLVM intrnisic and to an AArch64 instruction, so I use a short name to refer to all three)

• ttest is set to have side affects, so it's not reordered with tstart and tcommit; it's OK to move memory reads/writes around it, so it's also IntrNoMem; in that state tablegen would

set readnone attribute. Bu then for readnone:

On a function, this attribute indicates that the function computes its result (or decides to unwind an exception) based strictly on its arguments, without dereferencing any pointer arguments or otherwise accessing any mutable state (e.g. memory, control registers, etc)

and ttest *does* access mutable state. TBH, if the result of ttest is dead, there's no problem discarding the call, it just contradicts with the LLVM spec.

• It's also fine to reorder memory read/writes around irg; but it has actual side-effects, so it's not OK to discard it even if its result is read; if we define irg with IntrNoMem and IntrHasSideEffect it'll get readnone and might be discarded.

I'm not fine with the "contradicting LLVM spec" part.

Though, you and @arsenm convinced me that we, for now, need different control for intrinisic in the backend. That is, you want readnone set for them in t least right nowthe backend but not readnone set for them in the IR, correct? If that is so, this patch is fine with me (LGTM).

(For the record: I did initially not understand why one would like to have both, readnone and side-effects for an intrinsics. I still think that is wrong from the middle-end (IR) point of view, e.g., tcommit in IR has to carry a "write memory" effect, but it seems to be common in the backend.)

[jdoerfert]

Please make sure you add this only when you also add a test that makes sure the intrinsic annotation combo results in the desired IR attributes.

[chill]

Added a regression test.

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

+// RUN: llvm-tblgen -gen-intrinsic-impl -I %p/../../include %s | FileCheck %s
+
+// Get the minimum blurb necessary to process ...
+include "llvm/CodeGen/ValueTypes.td"
+include "llvm/CodeGen/SDNodeProperties.td"
+
+class LLVMType<ValueType vt> {
+  ValueType VT = vt;
+  int isAny = 0;
+}
+
+def llvm_i32_ty        : LLVMType<i32>;
+
+class IntrinsicProperty;
+def IntrNoMem : IntrinsicProperty;
+def IntrHasSideEffects : IntrinsicProperty;
+
+
+class Intrinsic<list<LLVMType> ret_types,
+                list<LLVMType> param_types = [],
+                list<IntrinsicProperty> intr_properties = [],
+                string name = "",
+                list<SDNodeProperty> sd_properties = []> : SDPatternOperator {
+  string LLVMName = name;
+  string TargetPrefix = "";
+  list<LLVMType> RetTypes = ret_types;
+  list<LLVMType> ParamTypes = param_types;
+  list<IntrinsicProperty> IntrProperties = intr_properties;
+  let Properties = sd_properties;
+
+  bit isTarget = 0;
+}
+
+// ... this intrinsic.
+def int_random_gen   : Intrinsic<[llvm_i32_ty], [], [IntrNoMem, IntrHasSideEffects]>;
+
+// CHECK: 1, // llvm.random.gen
+// CHECK: case 1:
+// CHECK-NEXT: Atts[] = {Attribute::NoUnwind}

llvm/utils/TableGen/CodeGenDAGPatterns.cpp

TreePatternNodePtr TreePattern::ParseTreePattern(Init *TheInit,
   if (Operator->isSubClassOf("Intrinsic")) {
     const CodeGenIntrinsic &Int = getDAGPatterns().getIntrinsic(Operator);
     unsigned IID = getDAGPatterns().getIntrinsicID(Operator)+1;
 
     // If this intrinsic returns void, it must have side-effects and thus a
     // chain.
     if (Int.IS.RetVTs.empty())
       Operator = getDAGPatterns().get_intrinsic_void_sdnode();
-    else if (Int.ModRef != CodeGenIntrinsic::NoMem)
+    else if (Int.ModRef != CodeGenIntrinsic::NoMem || Int.hasSideEffects)
       // Has side-effects, requires chain.
       Operator = getDAGPatterns().get_intrinsic_w_chain_sdnode();
     else // Otherwise, no chain.
       Operator = getDAGPatterns().get_intrinsic_wo_chain_sdnode();
 
     Children.insert(Children.begin(),
                     std::make_shared<TreePatternNode>(IntInit::get(IID), 1));
   }

llvm/utils/TableGen/IntrinsicEmitter.cpp

if (ae) {
         } while (ai != ae && intrinsic.ArgumentAttributes[ai].first == argNo);
         OS << "};\n";
         OS << "      AS[" << numAttrs++ << "] = AttributeList::get(C, "
            << attrIdx << ", AttrParam" << attrIdx << ");\n";
       }
     }
 
     if (!intrinsic.canThrow ||
-        intrinsic.ModRef != CodeGenIntrinsic::ReadWriteMem ||
+        (intrinsic.ModRef != CodeGenIntrinsic::ReadWriteMem && !intrinsic.hasSideEffects) ||
         intrinsic.isNoReturn || intrinsic.isCold || intrinsic.isNoDuplicate ||
         intrinsic.isConvergent || intrinsic.isSpeculatable) {
       OS << "      const Attribute::AttrKind Atts[] = {";
       bool addComma = false;
       if (!intrinsic.canThrow) {
         OS << "Attribute::NoUnwind";
         addComma = true;
       }
if (!intrinsic.canThrow ||
         if (addComma)
           OS << ",";
         OS << "Attribute::Speculatable";
         addComma = true;
       }
 
       switch (intrinsic.ModRef) {
       case CodeGenIntrinsic::NoMem:
+        if (intrinsic.hasSideEffects)
+          break;
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadNone";
         break;
       case CodeGenIntrinsic::ReadArgMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadOnly,";