In D52366#1269627, @kparzysz wrote:

The encoding of an instruction includes encodings of its operands. How are you planning to implement separate encoder/decoder methods? Right now their names are all embedded into operand definitions.

ping

ping

ping

LGTM with a nit. Also we should consider whether we ought to be calling isSafeToMove() when we're not moving things.

ping

ping

ping

ping

ping

LGTM

I haven't gone through the patch in full but I have a few comments based on the discussion we had today

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

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.

ping

ping

ping

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.

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.

Correct a silly mistake in fieldFromInstruction(). The bit of code that
actually built the mask in the APInt-like case was missing.

In D51489#1219289, @Petar.Avramovic wrote:

On MIPS 32 integer compare instruction are implicitly zero extended, meaning bitwise instructions (generated by legalizer) are superfluous. Where would be the best place to eliminate superfluous extends, perhaps upcoming prelegalize combiner ?

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.

Thanks

This seems like an obvious fix but I don't know whether it was deliberately or accidentally mandatory.

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

LGTM. I'm surprised the buildbots didn't report this sooner.

In D50272#1191922, @RKSimon wrote:

@dsanders I'm curious how you noticed this particular case - were you just going through perf traces, noticed the hit and manually worked out how the map was being poorly used?

Fixed most of the nits. Left out the one about iterating over DwarfRegNums since it would make the patch noisy.

Use std::move
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

Rebased to master. This is still ready to commit but it needs a corresponding change to ISel
to be able to land.

LGTM

Thanks Amara

LGTM :-)

LGTM with a note about clang-format

LGTM

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

LGTM

LGTM

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.