- User Since
- Aug 19 2013, 3:30 PM (269 w, 6 d)
Fri, Oct 19
Tue, Oct 16
LGTM with a nit. Also we should consider whether we ought to be calling isSafeToMove() when we're not moving things.
Mon, Oct 15
Fri, Oct 12
Fri, Oct 5
Thu, Oct 4
I haven't gone through the patch in full but I have a few comments based on the discussion we had today
Wed, Oct 3
This should be fixed by r343696. The problem was that -verify-machineinstrs=0 didn't work correctly for EXPENSIVE_CHECKS
Sure, I've managed to reproduce it. I should have a fix shortly
Tue, Oct 2
Mon, Oct 1
I believe the only remaining issue on this was the compile-time impact. I've run CTMark* before and after and found that no tests were significantly different. The geomean was improved by 1.24%. I'm going to be working on the combiner infrastructure for the next few months so if there are any further issues that come up we can address them post-commit.
Thu, Sep 27
Sorry for being so slow to get back to you on this. I hadn't realized I didn't say 'LGTM with a test case' or something to that effect.
Tue, Sep 25
Sep 21 2018
Add requirement for == and !=.
Add requirement for conversion to uint64_t. I'd like to remove this one again but it's currently necessary for field extraction
Remove conversion to bool. This proved to be very dangerous as it happily casts to any integral type via bool, silently truncating data.
Sep 17 2018
Correct a silly mistake in fieldFromInstruction(). The bit of code that
actually built the mask in the APInt-like case was missing.
Sep 14 2018
Aug 31 2018
Aug 23 2018
If required I can add a test that IRTranslation for above doesn't crash and produces ANYEXT. I haven't added as this seemed overkill for removing an assert.
Aug 21 2018
Aug 20 2018
This seems like an obvious fix but I don't know whether it was deliberately or accidentally mandatory.
Aug 14 2018
This LGTM. I'm ok with the G_INTRINSIC_TRUNC spelling for the trunc() intrinsic but we should reach an agreement on it before committing.
Fixed the various nits
Aug 12 2018
LGTM. I'm surprised the buildbots didn't report this sooner.
Aug 8 2018
Aug 7 2018
Fixed most of the nits. Left out the one about iterating over DwarfRegNums since it would make the patch noisy.
Aug 3 2018
Fix the comparison by pointer (but still avoid redundant use of LessRecordRegister since it's expensive compared to what we need)
Fix the lower_bound issue
Jul 30 2018
Jul 9 2018
Jun 27 2018
Jun 26 2018
Rebased to master. This is still ready to commit but it needs a corresponding change to ISel
to be able to land.
Jun 25 2018
Jun 21 2018
Jun 15 2018
Jun 8 2018
Jun 7 2018
May 30 2018
LGTM with a note about clang-format
May 29 2018
The full patch. The previous update was taken from the wrong machine and hadn't
been finished. Aside from not compiling, there was also a bug where additional
combine attempts would see G_SEXTLOAD mutate into G_LOAD (the anyext form).
Rewrite the patch such that the extends hoist up to the loads. The previous
patch had a couple problems that became clear on the bots:
- The loads could be duplicated. This is a correctness problem for volatile loads but more generally is likely to harm performance.
- The loads would sink to the extends without any hazard checking
Re-opening as I'm about to update it with a revised version. The previous one broke several bots because it sank loads down to the extends
Generally LGTM. I have a couple questions on this one though
May 8 2018
Having the side-effect inside the assert is harmless since the #ifndef ensures the whole loop is only built when asserts are enabled and Index is only used by the assert. However, I agree it's weird to have side-effects inside the assert. Moving the increment out of the assert and into the for loop would LGTM
Yes, I do have my doubts that the original implementation with sign extension is always correct, but it's NFC here anyway. I'm thinking to take a look at it at some point and see if I can come up with a breaking test case for that sign extend.