- User Since
- Dec 4 2014, 6:14 AM (302 w, 2 d)
Apr 23 2018
@Craig what do you think?
Feb 5 2018
Feb 1 2018
Thanks for taking care of this.
Jan 31 2018
Jan 30 2018
Jan 28 2018
You are right, I forgot that instcombine algorithm can handle zext/sext with multi use in case they have same type as trunc instruction destination type.
Here is a better test suggestion:
Jan 25 2018
Jan 24 2018
If there is no more concerns, can I get approval?
Jan 23 2018
Moved the aggressive-inst-combine pass to run with -O3.
I prefer to make this change now, in order to get approval to commit the pass, and in the future, once the pass is complete, to argue enabling it with -O2, in a separate discussion.
Jan 14 2018
Jan 6 2018
I uploaded a new version about a week ago with the required change for not generating new vector type.
Please, let me know if you have any other comments.
Dec 26 2017
Dec 21 2017
Addressed Craig and Sanjay comments:
- Retrieve the support for vector types.
- Make sure that this transformation will not create a new vector type. This is achieved by allowing reducing expression with vector type only when MinBitWidth == TruncBitWidth.
Thanks for Zvi for helping me progress with this review while I am on vacation.
I will continue as an author from here.
Dec 19 2017
Dec 18 2017
Thanks Zvi for addressing all comments and questions while I am away.
Craig, please, see answers for your questions inlined below.
Nov 2 2017
Minor typo update.
Minor fix, forgot to use IRBuilder in one case in the previous patch.
Addressed Zvi's Comments.
Thanks Zvi for the comments.
I will upload a new patch with most of the comments fixed.
See few answers below.
Nov 1 2017
Addressed Hal's comments.
Thanks Hal for the review.
I will update the patch with the changes you ask for.
Also, see one answer below.
Oct 30 2017
Separated the implementation from InstCombine pass and introduced a new pass called AggressiveInstCombine, which is called only twice (compared InstCombine, which is called ~6 times), one time as part of function simplification passes and second time as part of LTO optimization passes.
Oct 26 2017
Addressed Zvi's comments.
Thanks Zvi for the comments.
I fixed most of them and will upload a new patch soon.
Oct 25 2017
Answered David's comment.
Updated patch according to Craig comment. (Fixed minor logical bug)
Oct 18 2017
Oct 15 2017
Addressed Craig and Zia comments.
Thanks Craig and Zia for the review and sorry for the late answer.
Please, see answers below.
Oct 3 2017
Do we have kill flags set? If so, we could move a use of a virtual register across its kill... But maybe there is some reason to assure that this is not the case.
Addressed Chandler comment.
Thanks Chandler for reviewing the code.
Oct 2 2017
Called "initializeX86CmovConverterPassPass" also from the pass constructor.
Addressed Craig comment.
Sep 28 2017
Sep 27 2017
New approach was uploaded to D38313.
Fixed "no new line at end of file" issue.
Sep 25 2017
Sep 6 2017
Sep 5 2017
So, how do you wish to proceed from here?
Do you think that such optimization should be moved to separate new pass? Though it will be doing very similar thing as InstCombine, just will catch more cases that it does today?
Aug 30 2017
Aug 29 2017
Fixed typo and naming according to Craig comments.
Thanks Craig for the comments.
Will upload an updated patch soon.
Aug 27 2017
fixed some typos.
Aug 25 2017
Aug 24 2017
These changes looks good to me.
Any more comments?
Aug 23 2017
Aug 22 2017
I have one comment below.
By the way, I noticed that the double AssertZero occur only for the x86_64 (in i386 it does not happen).
It might be worth checking where it comes from, regardless of this patch.
Aug 21 2017
Updated patch to address Craig's comment.
Aug 20 2017
Aug 19 2017
There is still an issue with this implementation, here another reproducer:
Aug 18 2017
Thanks Chandler for preparing the patch, the implementation looks elegant, however, it overlooked a case where the memory registers are a result of a previous CMOV instructions.
This is a small reproducer that result in bad MIR:
Aug 17 2017
After committing the other parts, this patch was reduced to handle only the multiply in ComputeNumSignBitsImpl.
Aug 16 2017
The code looks good, just few comments need to be fixed/added (see below)