Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EVM] Improvements #759

Open
wants to merge 35 commits into
base: ef-stackification
Choose a base branch
from

Conversation

vladimirradosavljevic
Copy link
Contributor

No description provided.

PavelKopyl and others added 30 commits December 5, 2024 14:19
ARGUMENT instructions should always be located at the beginning
of a MF`s entry basic block and be ordered in ascending order
of their operand values.
Original idea and some code parts were taken from
the Ethereum`s compiler (solc) stackification algorithm.
Signed-off-by: Vladimir Radosavljevic <[email protected]>
This patch adds pseudo jumps, call and ret instructions
to fix machine verifier after stackification and to
reduce complexity added with bundles.

Signed-off-by: Vladimir Radosavljevic <[email protected]>
Signed-off-by: Vladimir Radosavljevic <[email protected]>
Signed-off-by: Vladimir Radosavljevic <[email protected]>
Signed-off-by: Vladimir Radosavljevic <[email protected]>
- Removed EVMControlFlowGraph/EVMControlFlowGraphBuilder classes
- Functionality that analyzes machine CFG and provides query methods
  was moved to EVMMachineCFGInfo class
- Information about MBB terminators is represented in EVMMBBTerminatorsInfo
  class
- StackSlot, Stack and Operation definitions  were moved to EVMStackModel
- Replaced almost all the std::map/std::set with llvm counterparts in
  EVMStackLayoutGenerator
…es to be treated as loads (#99999)

This change avoids deleting `!willReturn` intrinsics for which the
return value is unused when building the SDAG. Currently, calls to
read-only intrinsics not marked with `IntrWillReturn` cannot be deleted
at the LLVM IR level but may be deleted when building the SDAG. 
These calls are unsafe to remove from the IR because the functions are
`!willReturn` and should also be unsafe to remove fromthe SDAG for
the same reason. This change aligns the behavior of the SDAG to that
of LLVM IR. This change also requires that intrinsics not have the
`Throws` attribute to be treated as loads for the same reason.
Signed-off-by: Vladimir Radosavljevic <[email protected]>
This way, we can share address space AliasAnalysis
implementation between the backends.

Signed-off-by: Vladimir Radosavljevic <[email protected]>
Signed-off-by: Vladimir Radosavljevic <[email protected]>
This way, we can share SHA3ConstFolding implementation
between the backends.

Signed-off-by: Vladimir Radosavljevic <[email protected]>
Currently, it is only allowed to have memmove where
src and dst are from address space 1 (HEAP). Since
MemCpyOptPass can change memmove to memcpy, allow
this case, and don't issue an error.

Signed-off-by: Vladimir Radosavljevic <[email protected]>
For EVM, transformations to shift are preferable.

Signed-off-by: Vladimir Radosavljevic <[email protected]>
Signed-off-by: Vladimir Radosavljevic <[email protected]>
vladimirradosavljevic and others added 5 commits January 17, 2025 16:22
…context instructions

Signed-off-by: Vladimir Radosavljevic <[email protected]>
Since these instructions are cheaper than move, it is
beneficial to rematerialize them.

Signed-off-by: Vladimir Radosavljevic <[email protected]>
This looks like a rather weird change, so let me explain why this isn't
as unreasonable as it looks. Let's start with the problem it's solving.

```
define signext i32 @overlap_live_ranges(ptr %arg, i32 signext %arg1) { bb:
  %i = icmp eq i32 %arg1, 1
  br i1 %i, label %bb2, label %bb5

bb2:                                              ; preds = %bb
  %i3 = getelementptr inbounds nuw i8, ptr %arg, i64 4
  %i4 = load i32, ptr %i3, align 4
  br label %bb5

bb5:                                              ; preds = %bb2, %bb
  %i6 = phi i32 [ %i4, %bb2 ], [ 13, %bb ]
  ret i32 %i6
}
```

Right now, we codegen this as:

```
	li	a3, 1
	li	a2, 13
	bne	a1, a3, .LBB0_2
	lw	a2, 4(a0)
.LBB0_2:
	mv	a0, a2
	ret
```

In this example, we have two values which must be assigned to a0 per the
ABI (%arg, and the return value). SelectionDAG ensures that all values
used in a successor phi are defined before exit the predecessor block.
This creates an ADDI to materialize the immediate in the entry block.

Currently, this ADDI is not sunk into the tail block because we'd have
to split a critical edges to do so. Note that if our immediate was
anything large enough to require two instructions we *would* split this
critical edge.

Looking at other targets, we notice that they don't seem to have this
problem. They perform the sinking, and tail duplication that we don't.
Why? Well, it turns out for AArch64 that this is entirely an accident of
the existance of the gpr32all register class. The immediate is
materialized into the gpr32 class, and then copied into the gpr32all
register class. The existance of that copy puts us right back into the
two instruction case noted above.

This change essentially just bypasses this emergent behavior aspect of
the aarch64 behavior, and implements the same "always sink immediates"
behavior for RISCV as well.
…ges for cheap instructions

Signed-off-by: Vladimir Radosavljevic <[email protected]>
Break critical edges in MachineSink optimizations for
instructions that are marked with isAsCheapAsAMove in
tablegen.

Signed-off-by: Vladimir Radosavljevic <[email protected]>
@akiramenai
Copy link
Collaborator

@vladimirradosavljevic is there a reason to merge it to ef-stackification rather than main?

@vladimirradosavljevic
Copy link
Contributor Author

@vladimirradosavljevic is there a reason to merge it to ef-stackification rather than main?

I tested this and regenerated tests against ef-stackification, so we don't need to do it when we merge ef-stackification. My suggestion is to merge this after ef-stackification is merged into main. Wdyt?

target datalayout = "E-p:256:256-i256:256:256-S256-a:256:256"
target triple = "evm"

declare void @use(i256)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though compilation to asm works well here, attempt to emit an object file will cause a crash. That's because for EVM we do not support compilation units.
I think it's better to define @use as an empty, no-inline function at the point of a future work on assembler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't know about that, thanks for this info. Even though we have this issue with emitting an object files, do you think it is better to change that over simplicity in tests? Do you think we will use some of these tests for future work on assembler?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we may use these files for asm parser testing, but ok let's leave them as is. It's not a bit deal to change when need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants