-
Notifications
You must be signed in to change notification settings - Fork 0
[MLIR][OpenMP] Updates to initial Taskloop Bounds Implementation #3
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
base: kaviya_taskloop
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -395,18 +395,19 @@ Value *createFakeIntVal(IRBuilderBase &Builder, | |||||||||
| OpenMPIRBuilder::InsertPointTy OuterAllocaIP, | ||||||||||
| llvm::SmallVectorImpl<Instruction *> &ToBeDeleted, | ||||||||||
| OpenMPIRBuilder::InsertPointTy InnerAllocaIP, | ||||||||||
| const Twine &Name = "", bool AsPtr = true) { | ||||||||||
| const Twine &Name = "", bool AsPtr = true, bool Is64Bit = false) { | ||||||||||
|
||||||||||
| const Twine &Name = "", bool AsPtr = true, bool Is64Bit = false) { | |
| const Twine &Name = "", bool AsPtr = true, IntegerType *IntTy) { | |
| Builder.restoreIP(OuterAllocaIP); | |
| IntTy = IntTy ? IntTy : Builder.getInt32Ty(); |
More flexible.
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.
Done
Outdated
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.
You could do this from IntTy with llvm::ConstantInt::get or IRBuilderBase::getIntN
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.
Done with ConstantInt::get
Outdated
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.
| "lb", false, true); | |
| "lb", /*AsPtr=*/false, Builder.getInt64Ty()); |
It isn't obvious what the bool values mean without some extra help
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.
Done
Outdated
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.
nit: llvm style is to use C++ style comments
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.
Done
Outdated
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.
nit: comment style
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.
Done
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.
| divideCeil(M.getDataLayout().getTypeSizeInBits(Taskloop), 8)); | |
| divideCeil(M.getDataLayout().getTypeSizeInBits(Task), 8)); |
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.
| divideCeil(M.getDataLayout().getTypeSizeInBits(Taskloop), 8)); | |
| divideCeil(M.getDataLayout().getTypeSizeInBits(Task), 8)); |
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.
As we are utilizing structArg to store the the loopbound values, the struct __OMP_STRUCT_TYPE(Taskloop, kmp_task_info, false, VoidPtr, VoidPtr, Int32, VoidPtr, VoidPtr, Int64, Int64, Int64) is no longer needed.
The required size for storing loop bounds can be reserved in kmp_task_t by strutArg itself.
Outdated
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 could be moved above line 2035 to make that section clearer
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've moved this above the declaration for ArgStructAlloca so this can use the Shareds variable rather than calling the getArgOperand function
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.
| Builder.CreateStore(CastedLBVal, Lb); | |
| auto *Idx0 = Builder.getInt32(0); | |
| llvm::Value *Lb = Builder.CreateGEP(ArgStructType, TaskShareds, {Idx0, Builder.getInt32(0)}); |
The values of lb,ub and step are already populated in StructArg. You can directly access it and pass the pointer to the runtime call __kmpc_taskloop(...)
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.
| llvm::Value *Ub = Builder.CreateStructGEP(ArgStructType, TaskShareds, 1); | |
| llvm::Value *Ub =Builder.CreateGEP(ArgStructType, TaskShareds, {Idx0, Builder.getInt32(1)}); |
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.
GEP to StructArg and get the upper bound value.
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.
Same here. Remove the store instruction.
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.
| llvm::Value *Step = Builder.CreateStructGEP(ArgStructType, TaskShareds, 2); | |
| llvm::Value *Step =Builder.CreateGEP(ArgStructType, TaskShareds, {Idx0, Builder.getInt32(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.
Remove the store.
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.
It would also be good to check that the value being indexed is the right one, not just the numeric value of the index.
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.
Added a check to make sure the GEP Instruction being checked is using the Shared's as its first operand.
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.
Tom raised a concern that this add instruction pattern might also match other unrelated add instructions, and we discussed this in my PR: llvm#166903 (comment)
He suggested looking at the wsloop and distribute implementations for guidance on how this is handled there. I have not had a chance to dig into that yet. Could you please check this once?
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.
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.
extractCodeRegionexpects there to be a SetVector for both Inputs and Outputs. The API gives 2 options, one where you just pass the CEAC value, and another that includes the inputs and outputs. I am happy to exclude the Outputs from the OutlineInfo struct, but there will need to be a SetVector made before extracting the code region from OpenMPIRBuilder::finalize.