[TableGen] Make `!and` and `!or` short-circuit #113963

mshockwave · 2024-10-28T21:30:03Z

By preemptively simplifying the result of !and and !or, we can fold some of the conditional operators, like !if or !cond, as early as possible.

By preemptively simplifying the result of `!and`, we can fold some of the conditional operators, like `!if` or `!cond`, as early as possible.

llvmbot · 2024-10-28T21:30:42Z

@llvm/pr-subscribers-tablegen

Author: Min-Yih Hsu (mshockwave)

Changes

By preemptively simplifying the result of !and, we can fold some of the conditional operators, like !if or !cond, as early as possible.

Full diff: https://github.com/llvm/llvm-project/pull/113963.diff

2 Files Affected:

(modified) llvm/lib/TableGen/Record.cpp (+17)
(modified) llvm/test/TableGen/true-false.td (+14)

diff --git a/llvm/lib/TableGen/Record.cpp b/llvm/lib/TableGen/Record.cpp
index 1d71482b020b22..1f01b82685718a 100644
--- a/llvm/lib/TableGen/Record.cpp
+++ b/llvm/lib/TableGen/Record.cpp
@@ -1543,6 +1543,23 @@ const Init *BinOpInit::resolveReferences(Resolver &R) const {
   const Init *lhs = LHS->resolveReferences(R);
   const Init *rhs = RHS->resolveReferences(R);
 
+  if (getOpcode() == AND) {
+    // Short-circuit. Regardless whether this is a logical or bitwise
+    // AND.
+    if (lhs != LHS)
+      if (const auto *LHSi = dyn_cast_or_null<IntInit>(
+              lhs->convertInitializerTo(IntRecTy::get(getRecordKeeper())))) {
+        if (!LHSi->getValue())
+          return LHSi;
+      }
+    if (rhs != RHS)
+      if (const auto *RHSi = dyn_cast_or_null<IntInit>(
+              rhs->convertInitializerTo(IntRecTy::get(getRecordKeeper())))) {
+        if (!RHSi->getValue())
+          return RHSi;
+      }
+  }
+
   if (LHS != lhs || RHS != rhs)
     return (BinOpInit::get(getOpcode(), lhs, rhs, getType()))
         ->Fold(R.getCurrentRecord());
diff --git a/llvm/test/TableGen/true-false.td b/llvm/test/TableGen/true-false.td
index 597ad9f5ecc8e7..a9094884dbdb9f 100644
--- a/llvm/test/TableGen/true-false.td
+++ b/llvm/test/TableGen/true-false.td
@@ -67,6 +67,20 @@ def rec7 {
   bits<3> flags = { true, false, true };
 }
 
+// The `!and` should be short-circuit such that `!tail` on empty list will never
+// be evaluated.
+// CHECK: def rec8
+// CHECK:   list<int> newSeq = [];
+// CHECK:   list<int> newSeq2 = [];
+
+class Foo <list<int> seq = []> {
+  bit containsStr = !ne(!find(NAME, "BAR"), -1);
+  list<int> newSeq  = !if(!and(!not(!empty(seq)), containsStr), !tail(seq), seq);
+  list<int> newSeq2 = !if(!and(containsStr, !not(!empty(seq))), !tail(seq), seq);
+}
+
+def rec8 : Foo<>;
+
 #ifdef ERROR1
 // ERROR1: Record name '1' is not a string

jurahul · 2024-10-28T23:42:57Z

llvm/lib/TableGen/Record.cpp

@@ -1543,6 +1543,23 @@ const Init *BinOpInit::resolveReferences(Resolver &R) const {
  const Init *lhs = LHS->resolveReferences(R);
  const Init *rhs = RHS->resolveReferences(R);

+  if (getOpcode() == AND) {


Do we need similar short circuit for OR as well?

Good point, I had thought this before. The problem is that since !or is for both logical and bitwise operation, we can't short circuit something like this !or(true, <unknown / unresolved>) until the second operand is resolved (in which case it's not a short circuit anymore) because we're not sure if the second operand is a int (bitwise OR) or a bit (logical OR).

That said, I think a separate logical OR / AND like you suggested might help us to apply short-circuit with more confidence.

Good point, I had thought this before. The problem is that since !or is for both logical and bitwise operation, we can't short circuit something like this !or(true, <unknown / unresolved>) until the second operand is resolved (in which case it's not a short circuit anymore) because we're not sure if the second operand is a int (bitwise OR) or a bit (logical OR).

I just made a blunder and forgot that !or(<all ones>, ...) can also be short-circuited. I'll add this case into this patch as well.

The short-circuit for !or against all ones was just added.

Paul-C-Anagnostopoulos · 2024-10-29T00:43:32Z

!and and !or are bitwise operators that take an arbitrary number of arguments. Given that they are not documented to short-circuit, isn't this an incompatible change?

(It's been awhile, so my memory may be failing me.)

jurahul · 2024-10-29T02:00:49Z

I had that concern as well, but I thought these lhs and rhs expressions are not side effecting (modulo error checkin). Or do you propose adding short circuit variants?

…

On Mon, Oct 28, 2024 at 5:43 PM Paul C. Anagnostopoulos < ***@***.***> wrote: !and and !or are bitwise operators that take an arbitrary number of arguments. Given that they are not documented to short-circuit, isn't this an incompatible change? (It's been awhile, so my memory may be failing me.) — Reply to this email directly, view it on GitHub <#113963 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APRMUB354BKDXHUOA3UPSHTZ53K4VAVCNFSM6AAAAABQYLEIAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBSHEZTQOBQGE> . You are receiving this because your review was requested.Message ID: ***@***.***>

Paul-C-Anagnostopoulos · 2024-10-29T15:03:42Z

Yes, it may be that all the operators that return integers or booleans have no side-effects, but that seems like something we should not rely on. I'm tempted to suggest that we add !land and !lor instead, which would take only bit arguments. But that isn't very C++-ish.

I'll leave the final decision to someone with a stronger opinion on the matter.

mshockwave · 2024-10-29T17:15:21Z

all the operators that return integers or booleans have no side-effects, but that seems like something we should not rely on

Is there a case where side effects might be produced in a (bang operator) expression? To my best understandings bang operators are all designed to be very..."Lisp" without any side effect. And even if we intermix other TableGen constructions into the expression, like !and(SomeClass<v>.field, ...) , I can't think of a case where side effects are produced from those constructions.

More generally speaking, given the fact that our current !or and !and are for both bitwise and logical, I agree that short-circuit behavior should not be generally applicable to them. But !and with one of the operands being zero is the case that will always work regardless of being logical or bitwise (assuming we have no side, of course, which I'd commented above). That's why this patch is relatively specific to !and.

we add !land and !lor instead, which would take only bit arguments. But that isn't very C++-ish.

@Paul-C-Anagnostopoulos could you elaborate a little more on this point? I thought C/C++ also have separate logical and bitwise operators.

mshockwave · 2024-10-29T17:45:06Z

To give a little more background, this is the case (which is also the attached test case) that motivates me to come up with this patch:

class Foo <list<int> seq = []> {
   bit containsStr = !ne(!find(NAME, "BAR"), -1);
   list<int> newSeq  = !if(!and(!not(!empty(seq)), containsStr), !tail(seq), seq);
 }

 def rec8 : Foo<>;

Since the !if condition cannot be resolved immediately, TableGen resolver actually resolve both !tail(seq) and seq right away, and trigger an assertion in the process because !tail doesn't like seq being empty.

There are two ways to solve this problem: (1) Fix !if to postpone the resolution of its true and false values (2) make !and short-circuit.

I post this patch to do (2) first because I thought it can benefit more cases in addition to !if conditions. I'm thinking of posting a patch to do (1) as well and I'm happy to know what's your thought on that.

Paul-C-Anagnostopoulos · 2024-10-29T17:48:18Z

Sheesh, of course you are right. C++ has both bitwise and logical operators. See what happens when I don't code any C/C++ for a year? It all goes right out of my brain.

I believe you are correct that no bang operators produce side-effects. So therefore it should be okay to short-circuit !and when a 0 argument is encountered, and !or when an all-1s argument is encountered.

Be sure to document this, or otherwise there is no point in doing it.

Or, in keeping with C++, we could leave !and and !or alone and add !land and !lor specifically for this purpose. They would accept only bit arguments. If we go this route, I'd add !lxor, too, even though it doesn't short-circuit.

optimisan · 2024-10-30T12:09:46Z

There are two ways to solve this problem: (1) Fix !if to postpone the resolution of its true and false values (2) make !and short-circuit.

I post this patch to do (2) first because I thought it can benefit more cases in addition to !if conditions. I'm thinking of posting a patch to do (1) as well and I'm happy to know what's your thought on that.

(1) would be very helpful imo
I was gonna take up (1) as well; I have a sort of workaround here because !if crashes here #107829 (comment) although this patch unifies the two-step resolution.

mshockwave · 2024-10-30T17:36:55Z

I have a sort of workaround here because !if crashes here #107829 (comment) although this patch unifies the two-step resolution.

Thanks for pointing out. I was actually thinking of a simple fix which removes the immediate resolutions of both operand in !if, but it seems like you have tried it and observed stack overflow (infinite recursive function calls)?

optimisan · 2024-10-30T18:25:54Z

I haven't tried removing the immediate resolution yet, but unifying the resolution works around this bug in !if - but only for the case of instantiating anonymous records (not for def with subclasses).
I think fixing !if should be the right way to go next.

mshockwave · 2024-10-30T19:00:45Z

I believe you are correct that no bang operators produce side-effects. So therefore it should be okay to short-circuit !and when a 0 argument is encountered, and !or when an all-1s argument is encountered.

Be sure to document this, or otherwise there is no point in doing it.

I just added short-circuit for !or and the corresponding documentations

optimisan

LGTM atleast;
maybe others will review?

optimisan · 2024-11-01T11:05:03Z

llvm/test/TableGen/true-false.td

+class Foo <list<int> seq = []> {
+  bit containsStr = !ne(!find(NAME, "BAR"), -1);
+  list<int> newSeq  = !if(!and(!not(!empty(seq)), containsStr), !tail(seq), seq);
+  list<int> newSeq2 = !if(!and(containsStr, !not(!empty(seq))), !tail(seq), seq);
+  list<int> newSeq3 = !if(!or(containsStr, -1), seq, !tail(seq));
+  list<int> newSeq4 = !if(!or(-1, containsStr), seq, !tail(seq));
+}


Is it better to have a simpler test since it's supposed to only check if !and/!or is shorting properly?
Something like this perhaps:

class Foo <int zero = 0, list<int> seq = []> { int unresolved = zero; list<int> andLHS = !if(!and(zero, unresolved), !tail(seq), seq); ... }

jurahul · 2024-11-04T18:18:17Z

llvm/lib/TableGen/Record.cpp

+  if (Opc == AND || Opc == OR) {
+    // Short-circuit. Regardless whether this is a logical or bitwise
+    // AND/OR.
+    // Ideally we could also short-circuit `!or(true, ...)`, but it's


The comment about !or now seems stale.

The comment about !or now seems stale.

It is up-to-date: in the case of !or, TableGen first casts every operands into int, which is a 64 integer underlying. A true is casted to 1 (rather than all ones) and because we only support short-circuit against all ones -- namely -1 -- a true is actually not short circuited. The comment that follows also clarifies why we can't short circuit against 1.

jurahul · 2024-11-04T18:22:51Z

llvm/lib/TableGen/Record.cpp

+          (Opc == OR && LHSi->getValue() == -1))
+        return LHSi;
+    }
+    if (const auto *RHSi = dyn_cast_or_null<IntInit>(


In C/C++, the short-circuit is only applied to the LHS of the operator. That is, if the RHS happens to be 0 for an &&, the LHS is still evaluated. This seems to diverge from that, in the sense if RHS is 0, the LHS won't be evaluated. Is that the intent? Or should we abide by C/C++ style short-circuit to reduce the element of surprise?

Or is this still under the umbrella that these evaluations do not produce side effects? I'd think even then, there may be value in just sticking with well known rules. I am assuming this is also sufficient for the cases you'd like to handle.

You have a good point here. My intention was that in the case of something like !and(<unresolved>, 0) we can avoid resolving the first operand, because it's futile.

may be value in just sticking with well known rules. I am assuming this is also sufficient for the cases you'd like to handle

Yeah I agree, I'll update the PR accordingly.

It's done now.

And cleanup the test

llvm/docs/TableGen/ProgRef.rst

jurahul · 2024-11-05T18:52:03Z

Change LGTM to me, so approving, but would like another approval from someone else as well.

The idea is that by preemptively simplifying the result of `!and` and `!or`, we can fold some of the conditional operators, like `!if` or `!cond`, as early as possible.

[TableGen] Make !and short-circuit when either of the operand is zero

425ddc6

By preemptively simplifying the result of `!and`, we can fold some of the conditional operators, like `!if` or `!cond`, as early as possible.

mshockwave requested review from Paul-C-Anagnostopoulos and jurahul October 28, 2024 21:30

llvmbot added the tablegen label Oct 28, 2024

jurahul reviewed Oct 28, 2024

View reviewed changes

Add short-circuit for !or and add documentations

8cce6ce

mshockwave changed the title ~~[TableGen] Make !and short-circuit when either of the operand is zero~~ [TableGen] Make !and and !or short-circuit Oct 30, 2024

mshockwave requested a review from jurahul October 31, 2024 17:23

optimisan reviewed Nov 1, 2024

View reviewed changes

jurahul reviewed Nov 4, 2024

View reviewed changes

Only short circuit against the left-most operand

b20cf29

And cleanup the test

jurahul reviewed Nov 5, 2024

View reviewed changes

llvm/docs/TableGen/ProgRef.rst Show resolved Hide resolved

jurahul approved these changes Nov 5, 2024

View reviewed changes

mshockwave merged commit e8b70e9 into llvm:main Nov 7, 2024
9 checks passed

mshockwave deleted the patch/tblgen-and-short-circuit branch November 7, 2024 18:22

[TableGen] Make !and and !or short-circuit #113963

[TableGen] Make !and and !or short-circuit #113963

Uh oh!

Conversation

mshockwave commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paul-C-Anagnostopoulos commented Oct 29, 2024

Uh oh!

jurahul commented Oct 29, 2024 via email

Uh oh!

Paul-C-Anagnostopoulos commented Oct 29, 2024

Uh oh!

mshockwave commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshockwave commented Oct 29, 2024

Uh oh!

Paul-C-Anagnostopoulos commented Oct 29, 2024

Uh oh!

optimisan commented Oct 30, 2024

Uh oh!

mshockwave commented Oct 30, 2024

Uh oh!

optimisan commented Oct 30, 2024

Uh oh!

mshockwave commented Oct 30, 2024

Uh oh!

optimisan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optimisan Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jurahul commented Nov 5, 2024

Uh oh!

Uh oh!

Uh oh!

[TableGen] Make `!and` and `!or` short-circuit #113963

[TableGen] Make `!and` and `!or` short-circuit #113963

mshockwave commented Oct 28, 2024 •

edited

Loading

mshockwave commented Oct 29, 2024 •

edited

Loading

optimisan left a comment •

edited

Loading

optimisan Nov 1, 2024 •

edited

Loading