[SandboxIR] Functions to find vectorizor-relevant properties #109221

Sterling-Augustine · 2024-09-18T23:53:25Z

When vectorizing, the destination type and value of stores is more relevant than the type of the instruction itself. Similarly for return instructions. These functions provide a convenient way to do that without special-casing them everywhere, and avoids the need for friending any class that needs access to Value::LLVMTy to calculate it.

Open to better naming.

…ies. When vectorizing, the destintation type and value of stores is more relevant than the type of the instruction itself. Similarly for return instructions. These functions provide a convenient way to do that without special-casing them everywhere, and avoids the need for friending any class that needs access to Value::LLVMTy to calculate it. Open to better naming.

vporpo

Not a huge fan of having these functions as member functions of the Value/User class, but not super against it either. Their semantics are fairly simple, so they might as well be part of the IR.

vporpo · 2024-09-19T00:09:12Z

llvm/include/llvm/SandboxIR/SandboxIR.h

@@ -411,6 +411,11 @@ class Value {

  Type *getType() const;

+  /// \Returns the expected type of \p V. For most Values this is equivalent


the expected type "of \p V"
Perhaps "the expected type of this Value" ?

obviated by refactoring.

vporpo · 2024-09-19T00:14:08Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

@@ -875,6 +875,8 @@ OperandNo: 0
  EXPECT_TRUE(I0->hasNUses(1u));
  EXPECT_FALSE(I0->hasNUses(2u));

+  // Check Value.getExpectedType


Redundant comment?

vporpo · 2024-09-19T00:16:08Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

+  EXPECT_EQ(S0->getExpectedValue(),
+            cast<sandboxir::StoreInst>(S0)->getValueOperand());
+  EXPECT_EQ(Ret->getExpectedValue(),
+            cast<sandboxir::ReturnInst>(Ret)->getReturnValue());


We are not checking the case where we return void. I would add a second function in the IR, with a:

define void @bar() { ret void }

Add getNumBits.

vporpo · 2024-09-19T22:36:48Z

llvm/include/llvm/SandboxIR/SandboxIR.h

@@ -4019,6 +4019,42 @@ class Function : public Constant {
 #endif
 };

+// Collector for SandboxIR related convenience functions that don't belong in


Isn't it better to move the SandboxIRUtils into a sperate file perhaps in SandboxIR/Utils.h ?

Sterling-Augustine · 2024-09-19T22:48:06Z

Please take another look when you get a chance.

llvm/include/llvm/SandboxIR/SandboxIR.h

vporpo · 2024-09-19T23:23:25Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

+  const DataLayout &DL = M->getDataLayout();
+  // getNumBits for scalars
+  // float
+  EXPECT_EQ(sandboxir::SandboxIRUtils::getNumBits(F->getArg(0), DL),


Shouldn't we also be testing getNumBits() on instruction that have an "unexpected" type? Because for the rest of them getting num bits is as simple as DL.getTypeSizeInBits(V->getType()) ?

vporpo · 2024-09-19T23:24:46Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

+            DL.getTypeSizeInBits(Type::getFloatTy(C)));
+  EXPECT_EQ(sandboxir::SandboxIRUtils::getNumBits(F->getArg(1), DL),
+            DL.getTypeSizeInBits(Type::getDoubleTy(C)));
+  EXPECT_EQ(sandboxir::SandboxIRUtils::getNumBits(F->getArg(2), DL), 8u);


nit: Why check with a hard-coded number and not DL.getTypeSizeInBits(F->getArg(2)->getType()) as you did above.

That's a touch subtle, and maybe it doesn't matter that much. But calling through to getType will always give whatever bit size of whatever LLVM type just happens to underlie the SB type. So if there is some error in how those two match and get bit sizes, it won't be found here.

Hard coding it assures that not only do the bitsizes match, the value it returns is the expected one in known cases.

llvm/include/llvm/SandboxIR/SandboxIR.h

vporpo · 2024-09-20T16:11:14Z

llvm/include/llvm/SandboxIR/SandboxIR.h

@@ -4018,7 +4018,6 @@ class Function : public Constant {
  void dumpOS(raw_ostream &OS) const final;
 #endif
 };
-


Let's not modify this file.

vporpo · 2024-09-20T16:11:31Z

llvm/include/llvm/SandboxIR/Type.h


  // Friend all instruction classes because `create()` functions use LLVMTy.
 #define DEF_INSTR(ID, OPCODE, CLASS) friend class CLASS;
 #define DEF_CONST(ID, CLASS) friend class CLASS;
 #include "llvm/SandboxIR/SandboxIRValues.def"
-  Context &Ctx;
+      Context &Ctx;


clang-format ?

vporpo · 2024-09-20T16:12:14Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

@@ -938,7 +941,6 @@ define i32 @foo(i32 %arg0, i32 %arg1) {
  Replaced = Ret->replaceUsesOfWith(I0, Arg0);
  EXPECT_TRUE(Replaced);
  EXPECT_EQ(Ret->getOperand(0), Arg0);
-


Undo line deletion.

vporpo · 2024-09-20T16:12:31Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

@@ -14,6 +14,7 @@
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Module.h"
+#include "llvm/SandboxIR/Utils.h"


Probably forgot to git add the header :P

vporpo

Just a couple of minor comments, looks good otherwise.

vporpo · 2024-09-20T20:05:03Z

llvm/include/llvm/SandboxIR/Utils.h

@@ -0,0 +1,52 @@
+//===- SandboxIR.h ----------------------------------------------*- C++ -*-===//


This should say "Utils.h" instead of "SandboxIR.h"

vporpo · 2024-09-20T20:05:48Z

llvm/include/llvm/SandboxIR/Utils.h

+//
+// Collector for SandboxIR related convenience functions that don't belong in
+// other classes.
+


Please add header guards just like in SandboxIR.h

vporpo · 2024-09-20T20:06:15Z

llvm/include/llvm/SandboxIR/Utils.h

+
+namespace llvm {
+
+namespace sandboxir {


nit: namespace llvm::sandboxir {

Done (incidentally, SandboxIR.h has it the other way.)

vporpo · 2024-09-20T20:07:22Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

+            DL.getTypeSizeInBits(Type::getFloatTy(C)));
+  EXPECT_EQ(sandboxir::Utils::getNumBits(F->getArg(1), DL),
+            DL.getTypeSizeInBits(Type::getDoubleTy(C)));
+  EXPECT_EQ(sandboxir::Utils::getNumBits(F->getArg(2), DL), 8u);


nit: since there is a // float comment above, perhaps this one needs a // integer comment? Or drop both ?

vporpo · 2024-09-20T20:23:24Z

llvm/include/llvm/SandboxIR/Utils.h

@@ -1,4 +1,4 @@
-//===- SandboxIR.h ----------------------------------------------*- C++ -*-===//
+//===- Utils.h ----------------------------------------------*- C++ -*-===//


Try to make this 80-columns wide

vporpo · 2024-09-20T20:24:19Z

llvm/include/llvm/SandboxIR/Utils.h

-} // namespace llvm
+} // namespace llvm::sandboxir
+
+#endif // LLVM_SANDBOXIR_TYPE_H


I think the comment should be // LLVM_SANDBOXIR_UTILS_H

github-actions · 2024-09-20T21:12:47Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Sterling-Augustine requested review from vporpo and slackito September 18, 2024 23:55

vporpo reviewed Sep 19, 2024

View reviewed changes

Move getExpectedType and getExpectedValue to a new utility class.

a744eb0

Add getNumBits.

vporpo reviewed Sep 19, 2024

View reviewed changes

Move to a separate header and address comments.

546d2bf

vporpo reviewed Sep 20, 2024

View reviewed changes

Address comments

e728e17

vporpo approved these changes Sep 20, 2024

View reviewed changes

Address comments.

b91bf61

vporpo reviewed Sep 20, 2024

View reviewed changes

Sterling-Augustine and others added 2 commits September 20, 2024 13:47

Final fixups

259f602

Merge branch 'main' into IrFixes

6c08ea0

github merge conflict resolution doesn't run clang-format.

b2eee38

Sterling-Augustine merged commit d1edef5 into llvm:main Sep 23, 2024
8 checks passed

Sterling-Augustine deleted the IrFixes branch October 7, 2024 21:14

		@@ -411,6 +411,11 @@ class Value {

		Type *getType() const;

		/// \Returns the expected type of \p V. For most Values this is equivalent

		@@ -0,0 +1,52 @@
		//===- SandboxIR.h ----------------------------------------------- C++ --===//

		@@ -1,4 +1,4 @@
		//===- SandboxIR.h ----------------------------------------------- C++ --===//
		//===- Utils.h ----------------------------------------------- C++ --===//

[SandboxIR] Functions to find vectorizor-relevant properties #109221

[SandboxIR] Functions to find vectorizor-relevant properties #109221

Uh oh!

Conversation

Sterling-Augustine commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vporpo left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Sterling-Augustine commented Sep 19, 2024

Uh oh!

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!

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!

vporpo left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sterling-Augustine commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024 •

edited

Loading