-
Notifications
You must be signed in to change notification settings - Fork 40
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
Migrate APInt attribute arguments to Builtin_IntegerAttr #222
Conversation
Fixes #204 |
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!
|
||
let builders = [ | ||
AttrBuilderWithInferredContext<(ins "APInt": $cmod, "Polynomial":$ideal), [{ | ||
return $_get(ideal.getContext(), cmod.getBitWidth(), cmod, ideal); | ||
AttrBuilderWithInferredContext< |
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.
do you end up using this builder? i was slightly concerned about the use of zextOrTrunc
here, so if it's unused (I don't see it in the diff, and it appears different) maybe best to remove it until tested or needed
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, maybe this is used when the parser sees the : i<width>
specified?
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.
So the builder can be used without parsing IR at all. It's just a function any old fool can call. If that fool (me) passes an IntegerType that is i32
but an APInt
with underlying 64 bits, then I believe anyone who later assk for the APInt back will still have it in 64 bits. This is supposed to ensure that the integer type and the APInt's bit width always match.
I am actually not sure which builders the parser uses...
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, yes when fixing my PR just now, I found out the default generated parser uses a builder (with IntegerAttr type param) like
LWEParamsAttr::get(odsParser.getContext(),
IntegerAttr((*_result_cmod)),
unsigned((*_result_dimension)));
and it ensured that the _result_cmod
was a parser that can gather the type attribute on the int.
This is supposed to ensure that the integer type and the APInt's bit width always match.
I guess what I'm asking is - do you need it right now if you have no callers?
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.
Fair enough, I thought I was using it. Removed.
d60b8a6
to
e10ad45
Compare
e10ad45
to
38eee4a
Compare
Fixes #221