-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add controlcode to transaction lowering. #840
Conversation
a8c6a57
to
32bac9b
Compare
@@ -6,15 +6,14 @@ | |||
|
|||
#include "iree-amd-aie/IR/AMDAIEOps.h" | |||
|
|||
#include <numeric> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused i think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used for std::accumulate
.
@@ -159,6 +159,10 @@ AMDAIEDeviceModel::AMDAIEDeviceModel( | |||
TRY_XAIE_API_FATAL_ERROR(XAie_TurnEccOff, &devInst); | |||
} | |||
|
|||
uint8_t AMDAIEDeviceModel::getAddressGranularityInBits() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bruh lol - this is the goofiest name for MinStrideBitWidth
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just named this in consistency with aie-rt/mlir-aie I believe, but yeah, it's goofy. Your suggestion is way better.. Updated it.
@@ -222,6 +222,10 @@ struct AMDAIEDeviceModel { | |||
/// aie-rt for whatever reason. Make sure the parameters can't be retrieved in | |||
/// another way before adding new fields to this struct. | |||
struct AMDAIEDeviceConfig { | |||
/// Set default addressing granularity to 32 bits as this is the value for | |||
/// all current architecture versions. | |||
uint8_t addressGranularityInBits{32}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea so this is unncessary today right? is the idea to plan ahead? i don't remember - is it shorter in aie4 or no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's mainly to avoid hardcoding this value everywhere else, or hide it inside the getter. I am trying to put all configs that can't be retrieved from aie-rt in this place.
let description = [{ | ||
This NPU controller operation patches the address inside the buffer | ||
descriptor with provided ID on the specified column. This enables codegen to | ||
provide an argument index and offset at compile time, which is then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I haven't been paying attention: arg index and offset to what? hal.buffer bindings or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg index is indeed the HAL buffer bindings and this way the firmware knows the index of the buffer to use, so it can retrieve the address. The offset that is provided will be added to that address.
`(` | ||
$tile `,` | ||
$value `,` | ||
`port_type` `=` $port_type `,` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would be strm_sw_port_type
to match StrmSwPortType
in aie-rt? Is that too long you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a bit long yeah, and also quite low level. Most people reading strm_sw_port_type
will need to look up what it means anyway.
let extraClassDeclaration = [{ | ||
/// Return the number of leading operands before the `offsets`, `sizes` and | ||
/// and `strides` operands. | ||
static unsigned getOffsetSizeAndStrideStartOperandIndex() { return 2; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be implemented for the OffsetSizeAndStrideOpInterface
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah duh whoops
ArrayRef<uint32_t> instructions = | ||
transactionBuilder.finalizeAndReturnInstructions(); | ||
workgroupOp->setAttr( | ||
"npu_instructions", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do me a favor - can you make this a real attr on that op? so that we're not doing string look-ups/matches/etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jup, done
amdaie.workgroup { | ||
amdaie.controlcode { | ||
amdaie.npu.address_patch {arg_idx = 0 : ui32, bd_id = 0 : ui32, col = 0 : ui32, offset = 0 : ui32} | ||
amdaie.end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know you're not defining amdaie.controlcode
in this PR but does this op really need a terminator? it doesn't have any control flow right? just "straight line"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I created this to replace the func.func
as used in aie.device
and didn't think much about whether it needs a terminator.
it doesn't have any control flow right?
No, it doesn't at this time, an isn't supported in the FW, but maybe in the (far) future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically LGTM - i'll probably take another look on monday but looks solid.
After you land this I think I'll take a break next week and port all this stuff to use the actual txn stuff in aie-rt. |
32bac9b
to
4a7318b
Compare
Ports the
AMDAIEDmaToNpuPass
which operates on AIE dialect toAMDAIE
dialect.NOTE: The
AMDAIEDmaToNpuPass
is kept around for the AIR flow.