ABCD dead branch elimination (upper bounds checks) #191

m-demare · 2025-05-15T18:36:12Z

No description provided.

m-demare · 2025-05-15T18:36:52Z

Tests will fail until pharo-ai/graph-algorithms#47 is merged

PalumboN

Nice!! 🔥

I leave some comments to improve and understand the implementation.

We should also add the dependecy to the AI repo, right?

PalumboN · 2025-05-19T08:28:47Z

Druid-Tests/DRPiABCDDeadBranchEliminationTest.class.st

+
+
+	self assert: (cfg blocks includes: b_end0).
+	self assert: (cfg blocks includes: b_end1) not.


Suggested change

self assert: (cfg blocks includes: b_end1) not.

self deny: (cfg blocks includes: b_end1).

PalumboN · 2025-05-19T08:31:23Z

Druid-Tests/DRPiABCDDeadBranchEliminationTest.class.st

+]
+
+{ #category : 'tests' }
+DRPiABCDDeadBranchEliminationTest >> testABCD2 [


What's the difference with testABCD1? Can we choose better names? :P

PalumboN · 2025-05-19T08:32:23Z

Druid-Tests/DRPiABCDDeadBranchEliminationTest.class.st

+	"             /->------------------------------------------------------------------------------------->-\
+	             / V--<-------------------<-\                                   /->---------------------/->--\
+	 |b_i| -> |b_while| -> |b_if1| -> |b_for| -> |b_if2| -> |b_if2_after_check1| -> |b_if2_after_check2| -> |b_end|
+                                           \-<------------------------------------------------------<-/
+	"
+
+	"Check Figure 3 in ABCD: Eliminating Array Bounds Checks on Demand for it to make *a bit* more sense"


PalumboN · 2025-05-19T08:32:59Z

Druid-Tests/DRPiNodesTest.class.st

@@ -23,9 +23,9 @@ DRPiNodesTest >> insertPiNodes: cfg [
 DRPiNodesTest >> optimize: cfg with: optimisations [

 	self insertPiNodes: cfg.
-
+	cfg inspect.


Suggested change

cfg inspect.

PalumboN · 2025-05-19T08:33:05Z

Druid-Tests/DRPiNodesTest.class.st

 	cfg applyOptimisation: ((optimisations collect: [ :o | o new ]) reduce: [ :o1 :o2 | o1 then: o2 ]).
-
+	cfg inspect.


Suggested change

cfg inspect.

PalumboN · 2025-05-19T08:55:43Z

Druid/DRABCDConstraintSolver.class.st

+	edges add: {
+			aDRValue operand asDRValue.
+			aDRValue.
+			0 }.
+
+	nodes add: aDRValue operand asDRValue.


Operands should be always DRValues 🤔

Suggested change

edges add: {

aDRValue operand asDRValue.

aDRValue.

0 }.

nodes add: aDRValue operand asDRValue.

edges add: {

aDRValue operand.

aDRValue.

0 }.

nodes add: aDRValue operand.

PalumboN · 2025-05-19T08:58:48Z

Druid/DRABCDConstraintSolver.class.st

+DRABCDConstraintSolver >> processPhi: aDRPhiFunction [
+
+	aDRPhiFunction operands do: [ :op |
+		nodes add: op.
+		edges add: {
+				op.
+				aDRPhiFunction.
+				0 } ]
+]


This code is similar to the copy, maybe we can make do something.

PalumboN · 2025-05-19T09:04:27Z

Druid/DRABCDConstraintSolver.class.st

+DRABCDConstraintSolver >> processPiNode: aDRPiNode [
+
+	| constraint |
+	constraint := aDRPiNode constraint.
+	nodes add: constraint constantValue asDRValue.
+
+	((constraint isKindOf: DRLessOrEqualsConstraint) or:
+		 (constraint isKindOf: DREqualsConstraint)) ifTrue: [
+		^ edges add: {
+				  constraint constantValue asDRValue. "According to the paper, the edge source should be the pi-node associated to the constraint,
+													but I don't think there's a difference here, and this is easier"
+				  aDRPiNode.
+				  0 } ].
+	(constraint isKindOf: DRLessConstraint) ifTrue: [
+		edges add: {
+				constraint constantValue asDRValue. "According to the paper, the edge source should be the pi-node associated to the constraint,
+													but I don't think there's a difference here, and this is easier"
+				aDRPiNode.
+				-1 "I'm assuming integers only, not sure if that's right"} ].
+]


I'm aware about all these type-checks to manage the constraints. Could we use polymorphism in some way?

I would like something like:

Suggested change

DRABCDConstraintSolver >> processPiNode: aDRPiNode [

| constraint |

constraint := aDRPiNode constraint.

nodes add: constraint constantValue asDRValue.

((constraint isKindOf: DRLessOrEqualsConstraint) or:

(constraint isKindOf: DREqualsConstraint)) ifTrue: [

^ edges add: {

constraint constantValue asDRValue. "According to the paper, the edge source should be the pi-node associated to the constraint,

but I don't think there's a difference here, and this is easier"

aDRPiNode.

0 } ].

(constraint isKindOf: DRLessConstraint) ifTrue: [

edges add: {

constraint constantValue asDRValue. "According to the paper, the edge source should be the pi-node associated to the constraint,

but I don't think there's a difference here, and this is easier"

aDRPiNode.

-1 "I'm assuming integers only, not sure if that's right"} ].

]

DRABCDConstraintSolver >> processPiNode: aDRPiNode [

| constraint constant distance |

constraint := aDRPiNode constraint.

constant := constraint constantValue asDRValue.

nodes add: constant.

distance := constraint distanceABCD.

edges add: {

constant. "According to the paper, the edge source should be the pi-node associated to the constraint, but I don't think there's a difference here, and this is easier"

aDRPiNode.

distance }.

]

Why here is using constraint constantValue, this should not work even between variables?

PalumboN · 2025-05-19T09:07:31Z

Druid/DRABCDConstraintSolver.class.st

+DRABCDConstraintSolver >> processValue: aDRValue [
+
+	aDRValue isAdd ifTrue: [ ^ self processAdd: aDRValue ].
+
+	aDRValue isSubtract ifTrue: [ ^ self processSub: aDRValue ].
+
+	aDRValue isCopy ifTrue: [ ^ self processCopy: aDRValue ].
+
+	aDRValue isPhiFunction ifTrue: [ ^ self processPhi: aDRValue ]
+]


Here there are also type checks.

PalumboN · 2025-05-19T09:09:02Z

Druid/DRConstantConstraintSolver.class.st

 { #category : 'testing' }
 DRConstantConstraintSolver >> isSatisfiable: aDRPiNode [
 	| collectedConstraint |
+
+	visited := Set new.


m-demare mentioned this pull request May 15, 2025

Add shortest path algorithm intentded to be used in ABCD algorithm in Druid pharo-ai/graph-algorithms#47

Merged

ABCD (upper bounds checks)

d6af43a

m-demare force-pushed the pi-nodes/abcd branch from 534222c to d6af43a Compare May 15, 2025 18:39

PalumboN reviewed May 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ABCD dead branch elimination (upper bounds checks) #191

ABCD dead branch elimination (upper bounds checks) #191

Uh oh!

m-demare commented May 15, 2025

Uh oh!

m-demare commented May 15, 2025

Uh oh!

PalumboN left a comment

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

PalumboN May 19, 2025

Uh oh!

Uh oh!



		self assert: (cfg blocks includes: b_end0).
		self assert: (cfg blocks includes: b_end1) not.

	self assert: (cfg blocks includes: b_end1) not.
	self deny: (cfg blocks includes: b_end1).

		cfg applyOptimisation: ((optimisations collect: [ :o \| o new ]) reduce: [ :o1 :o2 \| o1 then: o2 ]).

		cfg inspect.

ABCD dead branch elimination (upper bounds checks) #191

Are you sure you want to change the base?

ABCD dead branch elimination (upper bounds checks) #191

Uh oh!

Conversation

m-demare commented May 15, 2025

Uh oh!

m-demare commented May 15, 2025

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!