MLIR SROA #15

Moxinilian · 2023-04-18T11:05:34Z

Implementation of a simple form of generic SROA in MLIR.

mlir/include/mlir/Interfaces/MemorySlotInterfaces.h

mlir/lib/Transforms/Mem2Reg.cpp

gysit

Overall very nice!

I dropped a lot of comments and will need a second review to get more in depth with some parts of the algorithm. Especially some of the blocking uses handling was a bit confusing. And also the GEP stuff. I think it may make sense to add some more comments for the complicating things.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h

mlir/include/mlir/Interfaces/MemorySlotInterfaces.h

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

mlir/lib/Transforms/SROA.cpp

gysit · 2023-05-02T14:20:41Z

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

+        Returns whether all accesses in this operation to the provided slot are
+        done in a type-safe manner. To be type-safe, the access must only load
+        the value in this type as the type of the slot, and without assuming any
+        context around the slot. For example, a type-safe load must not load


I think it misses the inbounds aspect but we can keep it if you prefer the name.

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

mlir/include/mlir/Interfaces/MemorySlotInterfaces.h

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h

gysit · 2023-05-02T14:46:54Z

mlir/include/mlir/Transforms/Passes.td

@@ -214,6 +214,13 @@ def SCCP : Pass<"sccp"> {
  let constructor = "mlir::createSCCPPass()";
 }

+def SROA : Pass<"sroa"> {


This still needs some polishing :)

Tell me if what I wrote is satisfying.

Sounds good! Should we also add a pass flag that limits the size of the aggregates we consider for destruction (probably in terms of number of elements?).

mlir/include/mlir/Interfaces/MemorySlotInterfaces.h

definelicht · 2023-05-02T14:50:55Z

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h

@@ -198,6 +200,12 @@ class LLVMStructType

  LogicalResult verifyEntries(DataLayoutEntryListRef entries,
                              Location loc) const;
+
+  /// Destructs the struct into its indexed field types.


What does "destruct" mean here?

Let's go for destructure.

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

definelicht · 2023-05-02T14:57:07Z

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

I would explicitly mention which interfaces are SROA-related in the source code

So it's not super clear where the boundary is. There is "PromotableOpInterface" which is used by both, "TypeSafeOpInterface" which is used by SROA but has nothing SROA-specific to it. Generally I think those interfaces are meant to be used in any setting and happen to be used by mem2reg and SROA.

definelicht · 2023-05-02T14:57:58Z

mlir/include/mlir/Transforms/Mem2Reg.h

+  /// This is a DAG structure because an operation that must eliminate some of
+  /// its uses always comes from a request from an operation that must
+  /// eliminate some of its own uses.


omg this sentence 😂 Please add some structure to help parsing

Tell me if you prefer the new one.

definelicht · 2023-05-02T15:01:49Z

mlir/lib/Dialect/LLVMIR/IR/LLVMMemorySlot.cpp

How do I know what's new? 🥲

Blame Torvalds!

mlir/lib/Transforms/SROA.cpp

Dinistro

Added some NIT comments.

Dinistro · 2023-05-03T05:39:47Z

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h

+  Optional<DenseMap<Attribute, Type>> getDestructedLayout();
+
+  /// Returns which type is stored at a given integer index within the struct.
+  Type getTypeAtIndex(Attribute index);


At a first glance, it seems odd to use an Attribute type for index.

So the reason for this is that sometimes indices are not integers but fancy things. For example, if you wanted to use SROA on for example a hashmap dialect, your indices could be anything.

mlir/include/mlir/Interfaces/MemorySlotInterfaces.h

Dinistro · 2023-05-03T05:45:34Z

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

+    InterfaceMethod<[{
+        Returns the list of slots for which destruction should be attempted,
+        specifying in which way the slot should be destructed into subslots. The
+        subslots are indexed by attributes. This computes the type of the


NIT: "by integer/index attributes"?

Well no, because of what I mentioned above. I don't really know how to word it otherwise :/

Dinistro · 2023-05-03T05:47:01Z

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

+        may belong to a different allocator. The original slot must still exist
+        at the end of this call.
+
+        The builder is located at the beginning of the block where the slot


It seems a bit odd that the insertion point of the builder is part of the precondition.

I agree. This comes from suggestions upstream, though. I don't know if I should try to fight it too much.

Dinistro · 2023-05-03T05:47:36Z

mlir/include/mlir/Interfaces/MemorySlotInterfaces.td

+    InterfaceMethod<[{
+        Destructures this slot into multiple subslots. The newly generated slots
+        may belong to a different allocator. The original slot must still exist
+        at the end of this call.


The original slot must still exist at the end of this call.

Does this mean that destroy doesn't destroy the slot?

destruct destructs the slot but does not destroy it yes :)
I'm changing the name for destructure to make Johannes happy.

mlir/lib/Dialect/LLVMIR/IR/LLVMMemorySlot.cpp

Moxinilian · 2023-05-03T14:37:26Z

I'm going to squash all this and manually separate it into two different PRs. Feel free to finish the ongoing discussions here and move further comments to the new PRs once they are ready.

A case for this transformation, https://gcc.godbolt.org/z/nhYcWq1WE Fold mov w8, llvm#56952 movk w8, #15, lsl #16 ldrb w0, [x0, x8] into add x0, x0, 1036288 ldrb w0, [x0, 3704] Only LDRBBroX is supported for the first time. Fix llvm#71917

gysit reviewed Apr 18, 2023

View reviewed changes

mlir/include/mlir/Interfaces/MemorySlotInterfaces.h Outdated Show resolved Hide resolved

mlir/lib/Transforms/Mem2Reg.cpp Outdated Show resolved Hide resolved

Moxinilian force-pushed the mlir-sroa branch from 05fc606 to 6956a2b Compare April 20, 2023 11:48

Moxinilian requested a review from gysit April 24, 2023 09:30

Moxinilian force-pushed the mlir-sroa branch from 6956a2b to 6d0ca5a Compare April 27, 2023 14:30

Théo Degioanni added 15 commits April 27, 2023 16:48

first draft for SROA interfaces

0ab5c83

decouple mem2reg analysis and promotion

beb7682

address comments

b03a2e6

first draft of SROA pass structure

4e5475f

improve mem2reg interfaces for use with sroa

a4c4021

complete algorithm implementation

5e49c8f

adjust edge case behavior

e88b395

begin implementation of SROA interfaces for LLVM IR

805a4f4

adjust interfaces

5536c49

progress deeper SROA analysis

7dfdfe8

introduce type safety interface

06d4bdd

adjust interfaces + start LLVM IR implem

40ad9c7

finalize LLVM SROA implementation

2bd8bbc

add move

cd62893

remove loc from memory slot

14f666e

gysit reviewed Apr 28, 2023

View reviewed changes

Théo Degioanni added 5 commits April 28, 2023 17:02

beginning of SROA test suite

a4fce9a

add more tests

fc227c0

clarify gep-related code

07cc660

complete documentation

207e278

address comments

a28abf2

Moxinilian marked this pull request as ready for review May 2, 2023 14:41

Moxinilian requested review from gysit, definelicht and Dinistro May 2, 2023 14:41

gysit reviewed May 2, 2023

View reviewed changes

definelicht reviewed May 2, 2023

View reviewed changes

address some comments

8e6c3ec

Dinistro reviewed May 3, 2023

View reviewed changes

Théo Degioanni added 4 commits May 3, 2023 12:13

address a round of comments

3843f6e

address more comments

a843ed5

address more comments

6011b2b

rename destruct into destructure

5abecde

Moxinilian closed this May 16, 2023

MLIR SROA #15

MLIR SROA #15

Uh oh!

Conversation

Moxinilian commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gysit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dinistro 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Moxinilian commented Apr 18, 2023 •

edited

Loading