transformations: (licm) add can_be_hoisted helper function

[RRavikiran66]

Helper function can_be_hoisted for Loop invariant code motion. can_be_hoisted return True if the operation is invariant to be the Scf.For loop. I think the pytests can be improved a lot, I am not sure how to write pytests properly. So, I created below tests by seeing existing tests. If you can be suggest to good testcases, I will happily include those :)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.05%. Comparing base (bd14181) to head (c6aab48).
Report is 109 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3078      +/-   ##
==========================================
+ Coverage   89.90%   90.05%   +0.15%     
==========================================
  Files         418      435      +17     
  Lines       52902    54647    +1745     
  Branches     8202     8473     +271     
==========================================
+ Hits        47562    49214    +1652     
- Misses       4014     4060      +46     
- Partials     1326     1373      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[superlopuh]

There's just a single region, and we have all the ops in scope, so we can just test each of the ops directly

Suggested change

	for region in for_op.regions:
	for op in region.block.walk():
	if not isinstance(op, scf.Yield):
	assert can_be_hoisted(op3, region)
	assert can_be_hoisted(op1, region)
	assert can_be_hoisted(op2, region)
	assert can_be_hoisted(op3, region)
	assert not can_be_hoisted(f, region)

This code currently does not test that calling the op with yield returns False

[superlopuh]

This is now looking great, but the structure of the module is a little hard to read. The ImplicitBuilder regions can be nested, which will make it a little easier to figure out what's going on. Here is what I would suggest to do:

Suggested change

	ci0 = test.TestOp(result_types=[i32])
	cf7 = test.TestOp(result_types=[f32])
	cf8 = test.TestOp(result_types=[f32])
	ci10 = test.TestOp(result_types=[i32])
	ci1 = test.TestOp(result_types=[i32])
	co0 = test.TestOp(result_types=[i32])
	co1 = test.TestOp(result_types=[i32])
	coi = test.TestOp(result_types=[i32])
	block = Block(
	arg_types=(
	index,
	index,
	)
	)
	block1 = Block(
	arg_types=(
	index,
	index,
	)
	)
	region_inner = Region(block)
	with ImplicitBuilder(block) as (_arg0, _):
	op1 = test.TestOp(result_types=[i32])
	# Test operation inside the loop
	hello = test.TestOp([op1], result_types=[i32])
	test.TestOp([hello], result_types=[])
	# Floating-point addition inside the loop
	v0 = Addf(cf7, cf8)
	region = Region(block1)
	with ImplicitBuilder(block1) as (_arg1, _):
	scf.For(ci0, ci10, ci1, [], region_inner)
	for_op = For(co0, co1, coi, [], region)
	body0 = Block([cf7, cf8, for_op])
	region_outer = Region(body0)
	func_type = FunctionType.from_lists([], [])
	function = func.FuncOp("invariant_loop_dialect", func_type, region_outer)
	outer_region = Region([Block()])
	with ImplicitBuilder(outer_region):
	ci32, cf32 = test.TestOp(result_types=[i32, f32]).results
	outer_for = For(ci32, ci32, ci32, [], Region(Block(arg_types=[index, index])
	with ImplicitBuilder(outer_for.body) as (_outer_i, _):
	inner_for = For(ci32, ci32, ci32, [], Region(Block(arg_types=[index, index])
	with ImplicitBuilder(inner_for.body) as (_inner_i, _):
	no_args = test.TestOp(result_types=[i32])
	args_in_inner = test.TestOp([no_args], result_types=[i32])
	no_results = test.TestOp([args_in_inner], result_types=[])

I removed the arith operation to avoid depending on that dialect, which is not relevant to this test. With this refactor, it is clear that the nested region functionality is not tested. It would be great to add another two testop in the loop, one of which has a region that is self-contained, the other of which depends on something outside of the for loop. I'm also not sure that the nested for loops get us anything.

[superlopuh]

In general, could you please check that each of the conditions in the can_be_hoisted function are tested? I'm not 100% sure that this is the last missing example.

[RRavikiran66]

I have made the changes

[PapyChacal]

There are some possible PR improvements as mentionned by @superlopuh already, but the content now seems good to me!

[superlopuh]

Fantastic! Thank you very much for iterating on this, it was more complex than I thought