Change aie.device op from NoTerminator to SingleBlockImplicitTerminator #1936
Conversation
fifield
requested review from
AndraBisca,
jgmelber and
stephenneuendorffer
as code owners
python/dialects/aie.py
Outdated
| @@ -290,7 +290,7 @@ def __init__( | |||
| ) | |||
|
|
|||
| def get_name(self): | |||
| return self.result.get_name() | |||
| return self.sym_name.value if self.sym_name else "" | |||
There was a problem hiding this comment.
Good change. Before this, I had to have some_buf.get_name()[1:] somewhere in my code, I think because it contained the @ in front of the name. Good to be aware of this though in case it breaks other code anywhere (hopefully nobody did what I did).
test/python/tile_array.py
Outdated
| # CHECK: (1, 0) <Channel: buffer=MemRef(%buffer_3_2, memref<10x10xi32>) producer_lock=Scalar(%buffer_3_2_producer_lock = aie.lock(%tile_3_2) {sym_name = "buffer_3_2_producer_lock"}) consumer_lock=Scalar(%buffer_3_2_consumer_lock = aie.lock(%tile_3_2) {sym_name = "buffer_3_2_consumer_lock"})> | ||
| # CHECK: (1, 1) <Channel: buffer=MemRef(%buffer_3_3, memref<10x10xi32>) producer_lock=Scalar(%buffer_3_3_producer_lock = aie.lock(%tile_3_3) {sym_name = "buffer_3_3_producer_lock"}) consumer_lock=Scalar(%buffer_3_3_consumer_lock = aie.lock(%tile_3_3) {sym_name = "buffer_3_3_consumer_lock"})> | ||
| # CHECK: (0, 0) <Channel: buffer=MemRef(%33, memref<10x10xi32>) producer_lock=Scalar(%34 = "aie.lock"(%14) <{sym_name = "_producer_lock"}> : (index) -> index) consumer_lock=Scalar(%35 = "aie.lock"(%14) <{sym_name = "_consumer_lock"}> : (index) -> index)> | ||
| # CHECK: (0, 1) <Channel: buffer=MemRef(%36, memref<10x10xi32>) producer_lock=Scalar(%37 = "aie.lock"(%15) <{sym_name = "_producer_lock"}> : (index) -> index) consumer_lock=Scalar(%38 = "aie.lock"(%15) <{sym_name = "_consumer_lock"}> : (index) -> index)> |
There was a problem hiding this comment.
Just out of curiosity, what is happening here? Why is it fine for all these locks to have the same symbol name?
There was a problem hiding this comment.
Ah, good catch, I'll investigate. There's a difference in how the ir is getting printed, likely because it isn't considered "valid" yet without a terminator when the print()s happen. But there seems to be some difference in how names/symbols are resolved too which I don't fully understand.
Yes, go for it to upstream! |
There is a block merging pattern used by the greedy pattern rewriter which queries a values's parent block's terminator. If the value is an aie.buffer in an aie.device, the getTerminator() call will assert because aie.device has no terminator. Another fix is to add a check in the upstream code.
fifield
force-pushed
the
aie_device_end
branch
from
7278acf to
77ae155
Compare
Take into account Xilinx#1936
There is a block merging pattern used by the greedy pattern rewriter which queries a values's parent block's terminator. If the value is an aie.buffer in an aie.device, the getTerminator() call will assert because aie.device has no terminator. This is the root cause of the bug in #1755. Another fix would be to modify the upstream code to first check that the block is expected to have a terminator