-
Notifications
You must be signed in to change notification settings - Fork 136
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
[CIR][CodeGen] Support for _mm_prefetch . #1522
base: main
Are you sure you want to change the base?
Conversation
@@ -95,7 +95,16 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned BuiltinID, | |||
default: | |||
return nullptr; | |||
case X86::BI_mm_prefetch: { | |||
llvm_unreachable("_mm_prefetch NYI"); | |||
mlir::Location loc = mlir ::UnknownLoc::get(&getMLIRContext()); |
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.
The location is known here: you can use getLoc(E->getExprLoc())
to retrieve it.
auto C = dyn_cast<cir::IntAttr>( | ||
cast<cir::ConstantOp>(Ops[1].getDefiningOp()).getValue()); | ||
auto Loc = mlir::IntegerAttr::get( | ||
mlir::IntegerType::get(builder.getContext(), 64), C.getUInt()); |
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 probably forgot an & 0x3
after C.getUInt()
, and the integer type should be 32-bit rather than 64.
See OG:
Value *Locality = ConstantInt::get(Int32Ty, C->getZExtValue() & 0x3);
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.
Oh yes , I was just trying something and didn't remove it from the commit. Thanks.
mlir::UnitAttr Write = | ||
ReadWrite ? mlir::UnitAttr::get(&getMLIRContext()) : nullptr; | ||
return builder.create<cir::PrefetchOp>(loc, Ops[0], Loc, Write) | ||
->getResult(0); |
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.
cir::PrefetchOp
expects a void*
, but Ops[0]
may be some other pointer type or an array type. You need to add builder.createBitcast(...)
to convert between them.
Moreover, the PrefetchOp
doesn't have a result, so ->getResult(0)
will raise an error at runtime. But we can't return a nullptr
or {}
either, because the emitBuiltinExpr
function will think these return values mean "unimplemented". Not sure what to return.
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.
We have a cir::CastOp will implement using that 👍🏻
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.
@AdUhTkJm @bcardosolopes I am finding it a little hard to implement the CastOp by just looking at the tablegen definition and the implementation given at https://llvm.github.io/clangir/Dialect/ops.html#circast-circastop
def CastOp : CIR_Op<"cast",
[Pure,
DeclareOpInterfaceMethods<PromotableOpInterface>]> {
operation ::= `cir.cast` `(` $kind `,` $src `:` type($src) `)`
`,` type($result) attr-dict
I am not able to find any implementation can you suggest some source ??
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.
Also I had a doubt , the Ops[0] doesnt seem to be a pointer type
llvm::SmallVector<mlir::Value, 4U> Ops
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 can look at CIRGenBuilder.h
and CIRGenBaseBuilder.h
to find some useful functions. For example, to create a bitcast, obtain a builder and write builder.createBitcast(...)
.
The website doesn't give a full implementation of ops. Try looking at CIROps.td
. To see what the fields mean, Operation Definition Specification on MLIR website will be helpful. You might also refer to TableGen programmer's reference for TableGen syntax.
Ops[0]
is an mlir::Value
; itself isn't a type, but it has some type (you can get it with getType()
). In PrefetchOp
the value is required to have a void pointer 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.
Thanks ,will look into it 👍
this is what I was able to write by taking the reference of the
in clang/lib/CIR/CodeGen/CIRGenBuilder.h |
the Current implementation is
this is without a return type using a PrefetchOp now , and finally this was the first time i did not get a stack dump :)
the test file is :
But the generated CIR seems to be incorrect :( (if i am not wrong). |
I believe there is
Without an We still need to return a |
Yes there are several occurances of Anyways just making the file PR open for review to seek for advice . |
Thanks for the review @AdUhTkJm
This is orthogonal to the cast feedback |
This draft PR needs review
I have to confirm if the implementation in the clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp is correct.
also to add a proper FileCheck instruction in the clang/test/CIR/CodeGen/X86/builtins-x86.c .