User Details
- User Since
- Sep 16 2016, 11:47 AM (297 w, 10 h)
Yesterday
Tue, May 17
Thanks for addressing the changes. LGTM but, as Mahesh mentioned, it's probably worth getting someone with experience in TOSA to take a look.
Mon, May 16
Thanks for working on this, Rob! I really can't comment on the actual implementation from the TOSA perspective. However, I would suggest not enabling this by default. Architectures with full 64-bit support should do better with the 64-bit lowering. I can follow up with a patch to enable this for the RISC-V specific cases.
Wed, May 11
Apr 20 2022
Thanks!
Apr 19 2022
Thanks, Amy! In general, moving the select op before the operation makes more sense to me since the op might have side effects that would make the execution incorrect otherwise.
I hope we can replace all of this once we have the masking proposal in place.
Apr 1 2022
Mar 24 2022
Much better, thanks!
Mar 23 2022
Thanks for the contribution, Javier! Some comments inline.
LGTM, thanks!
Mar 10 2022
Mar 9 2022
I'll commit it tomorrow if no more comments. Thanks!
Addressed feedback.
Mar 7 2022
LGTM, thanks!
I'll let @bondhugula clarify but my impression about his comment in D120776 is that we should be able to have a single implementation that works for both ScfForOp and AffineForOp by using/extending interfaces like LoopLikeInterface? Maybe I got it wrong. In any case, adding a scf specific implementation is also great. Thanks!
Mar 4 2022
Thanks for the feedback! I addressed the trivial one. The cleanup/generalization would need more thought before we move forward. More info below.
I'll move forward with this if no more comments.
Addressed feedback.
Mar 3 2022
Thanks, Amy. LGTM once Uday's comments have been addressed.
Mar 2 2022
Thanks, Amy! I think this is a very interesting extension! Some comments
Thanks, Uday. A couple of comments that would need attention. LGTM. You can move forward after addressing them.
Feb 25 2022
Feb 23 2022
Just curious if F32 data movement would not just work for any 32-bit entity, but change looks good to me.
An internal test exposed a bug in the implementation. This new patch
fixes the issue (see 'areDimsTransposedIn2DSlice') and improves documentation.
Could you please take a quick look?
Feb 17 2022
Generalize the implementation even more by using shape_cast before and
after the transposition.
Feb 10 2022
Thanks, Uday. LGTM!
Jan 26 2022
Thanks, Aart!
Thanks, Aart! LGTM
Jan 14 2022
Thanks, Sergei! Much clearer now. I think your proposal makes more sense to me now. The problem is very specific to the way you are generating the code but it's clearing exposing some corner cases of the create_mask operation. I'll leave it to Aart, though, since he has much more context on these ops (I'm not even sure I understand why we have the constant and non-constant variant of this op so I can't fully understand the implications of this change).
Jan 13 2022
Thanks for the contribution, Sergei! I think I don't have enough experience with this op so I'll leave this to @nicolasvasilache.
How do you end up generating values that are out of the expected bounds of the mask?
Folding invalid values into valid ones could be surprising and maybe lead to silent bugs (?) but maybe it makes sense for this op. An alternative would be generating code in your use case to handle these out-of-bounds values and truncate them accordingly before they are passed to the create_mask op. I would useful if you could share a bit more about your use case.
LGTM. Just a minor comment.
Thanks, Rahul for contributing this! It's great that it can be integreated with the existing walk methods. LGTM. I'll leave the final approval to @rriddle.
Jan 11 2022
Jan 10 2022
Dec 22 2021
LGTM. Just a comment about the test.
Dec 16 2021
Dec 15 2021
Addressed the feecback. The data layout spec is now passed using a string.
Much better. Thanks for the feedback!
Dec 14 2021
Dec 10 2021
Thanks!
Dec 1 2021
Thanks for helping with the 0-d vector problem! Added some minor comments. I have a question about the lowering of 0-d vectors to the scalar world. See comments inline.
Nov 29 2021
Thanks, Uday. LGTM but I'm not super familiar with the loop unrolling pass. I'll defer the approval to someone with more expertise in the pass.
Nov 23 2021
LGTM. Just one minor question. Thanks!
Nov 22 2021
Dropping FMF doesn't seem to be needed after all. See D111846
Dropping FMF doesn't seem to be needed after all. See D111846
Thanks for all the feedback! I think this will cover a good part of the cases that could actually go wrong in practice. We can accommodate more cases as we see fit.
- Maybe it's worth adding this link to the commit message: https://groups.google.com/g/llvm-dev/c/P_CXf5hPro8
- Typo? distinguishes between and intrinsics and an inline_asm implementation.
Thanks for addressing the feedback. LGTM. Just one minor comment. Feel free to fix it a commit it directly.
Nov 19 2021
LGTM, thanks!
I talked to Nuno in private and he mentioned that I could go ahead and commit the changes and address any minor feedback in a separate commit since he is very busy right now.
I'll commit this on Monday if no other comments.
Nov 18 2021
Thanks a lot for the fix! LGTM. Just a minor comment.
Nov 15 2021
Thanks, Florian! I addressed the feedback.
- Updated and added more comments.
- Simplified redundant condition.
Nov 12 2021
Any other comments? :)
Nov 11 2021
Nov 9 2021
Since it was already implemented in a previous version of the code, I restored the logic to drop flags
from Widen and WidenGEP recipes following Florian's suggestion. These changes only impact Florian's
test case. I modified the test case to also have a vector GEP feeding a masked load to also cover
that use case.
Added new test. Thanks, Florian!
Fixed naming convention issues
Nov 8 2021
Thanks, Nicolas! LGTM.
Ok, I'll wait for Florian's approval to land this patch.
Fixed failing test
Nov 7 2021
Thanks a lot for the contribution! LGTM. Just some minor comments.