User Details
- User Since
- Mar 16 2020, 6:35 AM (157 w, 4 d)
Today
It is a nice implementation dealing with all compilation stages, kudos for that! I am not in favor of extending TypeCategory with Vector, which is rather central in your patch after parsing, and I am suggesting an alternative (please wait for more feedback before spending too much time trying it though).
Yesterday
Thanks, this looks great to me.
Looks good to me, thanks for simplifying codegen and making transformations more modular.
Fri, Mar 17
LGTM
Thanks, LGTM. Please beware there is a bot failure, but I think it is unrelated to your patch (you could rebase if this was fixed to check that).
Tue, Mar 14
Looks good.
LGTM
LGTM
I think the cause of the issue mentioned in the second bullet point is that the patterns were actually not using the OpAdaptor adaptor to get the operands as they should have in a full translation pass (sorry for not catching that, I always miss this....). E.g: sum.getArray() sould have been getBufferizedExprStorage(adaptor.getArray()). And the hlfir::AsExprOp in processReturnValue could have been repaced by packageBufferizedExpr(loc, builder, *resultEntity, mustBeFreed). This would have avoided using/introducing again hlfir.expr in the pass that gets rid of them. This would also address the first point in some way (but I agree the hlfir.expr operands would still also be transformed).
Tue, Mar 7
LGTM
LGTM
Looks great, thanks.
Mon, Mar 6
Makes sense to me.
Looks good to me
Sun, Mar 5
LGTM
Fri, Mar 3
It makes sense to me to add this interface to fir.if and LGTM. Just adding @vzakhari here to ensure I am not missing something.
Thanks for having dealt with all the comments, LGTM
Thu, Mar 2
LGTM
Wed, Mar 1
LGTM
Looks good
Too bad, but not a blocker on my side. We will revisit pretty printing at some point to enable type alias usage for FIR derived types. Maybe we could see if it is possible to not add extra indentation to module content at that point.
Great job finding out the pieces, designator lowering is not the easiest piece of lowering. Looks good overall, some comment inline.
Thanks a lot, I think the IR can be slightly further simplify by merging the fir.absent and fir.emboxproc into a single fir.absent. Looks great otherwise.
Tue, Feb 28
Correct typo in verifier error message.
LGTM
LGTM
It's great to have a generic solution to deal with the clean-ups of construct entities when branching out of them, thanks Val.
Thanks Pete, LGTM.
adding it as a discussion point in one of the weekly meetings or is an RFC on discourse perhaps better?
We can briefly discuss it at the meeting tomorrow to check no one rely on not having the module printed.
Mon, Feb 27
LGTM
Nits in the result type computation, looks good otherwise.
Small nit, looks good otherwise.
Thanks a lot for the fix!
Sun, Feb 26
I think removing this check is not correct from a semantic point of view because we are required to gracefully catch double allocation errors when STAT/ERRMSG is provided.
Fri, Feb 24
Thu, Feb 23
LGTM
Wed, Feb 22
Remove genTempFromSourceBox that is now unused. Thanks Pete.