[SandboxIR] Implement BranchInst #100063

vporpo · 2024-07-23T05:56:28Z

This patch implements sandboxir::BranchInst which mirrors llvm::BranchInst.

BranchInst::swapSuccessors() relies on User::swapOperandsInternal() so this patch also adds Use::swap() and the corresponding tracking code and test.

This patch implements sandboxir::BranchInst which mirrors llvm::BranchInst. BranchInst::swapSuccessors() relies on User::swapOperandsInternal() so this patch also adds Use::swap() and the corresponding tracking code and test.

tschuett · 2024-07-23T05:59:35Z

llvm/include/llvm/SandboxIR/SandboxIR.h

+  bool isUnconditional() const {
+    return cast<llvm::BranchInst>(Val)->isUnconditional();
+  }
+  bool isConditional() const {


Copy-paste?

Do I need both?

Both of these function exist in llvm::BranchInst, so since we are trying to create a similar API I guess we should keep both.

Ignore that, I was too fast.

tschuett · 2024-07-23T06:01:18Z

llvm/include/llvm/SandboxIR/SandboxIR.h

+  }
+  Value *getCondition() const;
+  void setCondition(Value *V) { setOperand(0, V); }
+  unsigned getNumSuccessors() const { return 1 + isConditional(); }


number + bool?

return isConditional() ? 2 : 1;

I guess I could have casted it to unsigned first, but this is how it's done in llvm::BranchInst::getNumSuccessors() so I just copied it.

I am just afraid some compiler, including future Clang will complain.

I wouldn't worry too much about it, it's legal c++. Casting bool to int gives us 0 or 1.

Ignore me again.

It's always good to point out things that look strange or potentially wrong, so thanks for the comments.

tmsri · 2024-07-23T18:14:49Z

llvm/include/llvm/SandboxIR/SandboxIR.h

+                            Context &Ctx);
+  static BranchInst *create(BasicBlock *IfTrue, BasicBlock *InsertAtEnd,
+                            Context &Ctx);
+  static BranchInst *create(BasicBlock *IfTrue, BasicBlock *IfFalse,


Why can't IfFalse == nullptr be the one that creates the instruction with only the true branch? That way you save creating two functions or put it as a default parameter.

Yeah that would make sense, but I guess it is not as explicit as having a separate function. This is how it's done in llvm::BranchInst so I just created a similar API.

Sounds good!

tmsri

Slightly different from other instruction adds like ReturnInst and includes tracking, approving it.

tmsri · 2024-07-23T18:23:50Z

llvm/include/llvm/SandboxIR/SandboxIR.h

+                            Context &Ctx);
+  static BranchInst *create(BasicBlock *IfTrue, BasicBlock *InsertAtEnd,
+                            Context &Ctx);
+  static BranchInst *create(BasicBlock *IfTrue, BasicBlock *IfFalse,


Sounds good!

llvm-ci · 2024-07-23T18:56:25Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-win-fast running on as-builder-3 while building llvm at step 7 "test-build-unified-tree-check-llvm-unit".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/2919

Here is the relevant piece of the build log for the reference:

Step 7 (test-build-unified-tree-check-llvm-unit) failure: test (failure)
******************** TEST 'LLVM-Unit :: SandboxIR/./SandboxIRTests.exe/9/19' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\unittests\SandboxIR\.\SandboxIRTests.exe-LLVM-Unit-13192-9-19.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=19 GTEST_SHARD_INDEX=9 C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\unittests\SandboxIR\.\SandboxIRTests.exe
--

Script:
--
C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\unittests\SandboxIR\.\SandboxIRTests.exe --gtest_filter=SandboxIRTest.BranchInst
--
C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\SandboxIR\SandboxIRTest.cpp(688): error: Death test: Br->getCondition()
    Result: failed to die.
 Error msg:
[  DEATH   ] 

C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\SandboxIR\SandboxIRTest.cpp(700): error: Death test: Br->getCondition()
    Result: failed to die.
 Error msg:
[  DEATH   ] 


C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\SandboxIR\SandboxIRTest.cpp:688
Death test: Br->getCondition()
    Result: failed to die.
 Error msg:
[  DEATH   ] 

C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\SandboxIR\SandboxIRTest.cpp:700
Death test: Br->getCondition()
    Result: failed to die.
 Error msg:
[  DEATH   ] 



********************

This reverts commit 3993da2.

vporpo · 2024-07-23T19:03:26Z

Reverted: c312a1a the death tests need to be enabled only for the DEBUG build.

This reverts commit c312a1a.

vporpo · 2024-07-23T19:33:53Z

Reapplied: c444548

This patch implements sandboxir::BranchInst which mirrors llvm::BranchInst. BranchInst::swapSuccessors() relies on User::swapOperandsInternal() so this patch also adds Use::swap() and the corresponding tracking code and test.

Summary: This reverts commit 3993da2. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251030

Summary: This reverts commit c312a1a. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251151

[SandboxIR] Implement BranchInst

e98b2a2

This patch implements sandboxir::BranchInst which mirrors llvm::BranchInst. BranchInst::swapSuccessors() relies on User::swapOperandsInternal() so this patch also adds Use::swap() and the corresponding tracking code and test.

vporpo requested review from slackito, echristo, tschuett and tmsri July 23, 2024 05:56

tschuett reviewed Jul 23, 2024

View reviewed changes

tmsri reviewed Jul 23, 2024

View reviewed changes

tmsri approved these changes Jul 23, 2024

View reviewed changes

vporpo merged commit 3993da2 into llvm:main Jul 23, 2024
8 checks passed

vporpo added a commit that referenced this pull request Jul 23, 2024

Revert "[SandboxIR] Implement BranchInst (#100063)"

c312a1a

This reverts commit 3993da2.

vporpo added a commit that referenced this pull request Jul 23, 2024

Reapply "[SandboxIR] Implement BranchInst (#100063)"

c444548

This reverts commit c312a1a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SandboxIR] Implement BranchInst #100063

[SandboxIR] Implement BranchInst #100063

vporpo commented Jul 23, 2024

tschuett Jul 23, 2024

tschuett Jul 23, 2024

vporpo Jul 23, 2024 •

edited

Loading

tschuett Jul 23, 2024

tschuett Jul 23, 2024

tschuett Jul 23, 2024 •

edited

Loading

vporpo Jul 23, 2024

tschuett Jul 23, 2024

vporpo Jul 23, 2024

tschuett Jul 23, 2024

vporpo Jul 23, 2024

tmsri Jul 23, 2024

vporpo Jul 23, 2024

tmsri Jul 23, 2024

tmsri left a comment

tmsri Jul 23, 2024

llvm-ci commented Jul 23, 2024

vporpo commented Jul 23, 2024

vporpo commented Jul 23, 2024

[SandboxIR] Implement BranchInst #100063

[SandboxIR] Implement BranchInst #100063

Conversation

vporpo commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vporpo Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschuett Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmsri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvm-ci commented Jul 23, 2024

vporpo commented Jul 23, 2024

vporpo commented Jul 23, 2024

vporpo Jul 23, 2024 •

edited

Loading

tschuett Jul 23, 2024 •

edited

Loading