But I still believe that trying to spot the identical case in subranges is pointless because we have already proved that the values are identical in main ranges. It is impossible for the values to be not identical in subranges; it's just a bit tricky to fix up the code to demonstrate that.

Hi Aditya,

Hi Aditya,

I finally tracked down why I couldn't get a mir test before: Compiling a ll file as usual, arriving at RegisterCoalescer, the numbering of the MBBs was not quite in order. Using -stop-before names the MBBs in the mir according to those out-of-order numbers, but reading the mir in again with -run-pass fails to preserve it. Instead it renumbers the MBBs in order. RegisterCoalescing processes MBBs with the same loop depth and critical edgeness and connectedness in numbering order, so the pass behaved differently.

Could you elaborate on the exact constraints?

The fix here is to only detect an "identical erase" in the main range.

Could you elaborate on that part?

But it is not possible since can generate incorrect code in terms of exec mask.

FWIW, I am still not convince this is a good thing to match SDISel behavior at O0, but this is the least bad solution of doing that IMHO.

Hi Matthias,

Hi Amara,

Hi Amara,

Unfortunately I can only reproduce this in an ll test. Turning it into a mir test makes the problem go away.

Hi Tim,

Hi Tim,

If we agree that an input canonicalization pass is needed then doing this there adds much less complexity than teaching tablegen that G_ICMPs are special (or somehow adding legalisation rules to tablegen?).

So overall I think we should have some form of IR canonicalization before translation for the majority of cases to be handled for -O0.

It can't do that all the time, with these G_ICMPs for example it also has to swap the predicate (not that we even have imported patterns that could possibly match for AArch64).

Going back to a higher-level description for all that stuff, I wonder if ISel could already handle this.

how would it decide in which order to put those vregs in the generated instructions w/o explicitly checking for constants?

Does that mean that something after the last InstCombine produces icmp's with the non-canonical operand order? If so maybe that something needs to be chased down and fixed instead.

I agree with the motivation but I don’t think this is the right approach. I would prefer we run a canonicalization step even at O0 before GISel rather than duplicating this logic between IR and MI.
Jump tables are a different beast and have their place in here because they don’t really make sense to do on IR.

Hi Amara,

Thanks for your patience.
LGTM.
One final nit below. Feel free to commit directly.

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

Shouldn't we keep only the latest type and kind of extension?

LGTM.
Nitpicks on the comments.

Silly me for not catching this during the first review.

Good catch!

Either changing the default (trackLiveness == true by default) or printing it is fine.

Are you okay with us solving this just for STATEPOINTs at the moment by essentially just disabling remats for STATEPOINT uses?

Hi Puyan,

Hi Konstantin,

Do you agree we could drop this patch?

With the test case fixed

Up to you for the logging. We can keep it.

Doesn't the target already do this? At least there's a CoveredBySubRegs flag in the Register .td class ...

Would it work to instead use RegUnits, so that we first collect all saved/restored RegUnits, and then check which registers have all their RegUnits saved?

Nice finding Roman.

As you can see I left VMOVRRD ARM instruction as FIXME since it is more complicated copy.

E.g., on x86 saving xmm0 is not enough to say that ymm0 is preservered even if ymm0 doesn’t have any other sub reg.

Just reread what you said, you indeed want the other way.
The problem is that the big register is not necessarily covered by the sub regs. So you have to check for that.

I was thinking about this the other way around: if all the subreg of a register are saved that doesn’t imply the register itself is saved.
The way you describe with adding the sub regs works great. I thought you wanted the other way :P

Yes, please remove these.

Looks sensible to me.

The general direction makes sense and I agree you probably don't want to implement the full isEquivalentTo stuff I was talking about.
A general pointer you might have missed. We already track copy like instructions through isRegSequenceLike, isExtractSubRegLike and so on. These are more complicated than just plain copies though (e.g., VMOVRRD should already be supported).

This loop (per what determineCalleeSaves does) will only add any of the registers that getCalleeSavedRegs() returns

Nice!

Thanks for your patience, LGTM.

LGTM with the nits fixed

Thanks for double checking. Now, I don't think some part of the patch makes sense. See my inline comment.
Basically, we should have to augment whatever determineCalleeSaved gives us, otherwise that means this information is just flawed.

Good catch!

Hi Jonas,

With not llc you can still check specifics in FileCheck whereas with XFAIL you usually don't put checks at all.
In this case, I would expect a not llc + filecheck of some error message.

Couple of nits on the tests.

LGTM

Hi Jonas,

You're right, re-reading the comment indeed, not all the aliases are here (nor should they be!).

The new result isn't better than old result, neither worse. Do you think we should do transformation on this test case?

Hi Roman,

I am not sure I like the patch in the sense that the comment for UsedPhysRegsMask (in ’MachineRegisterInfo.h) explicitly said that it should contain all the aliases.

A while back, with Hal, we talked about having something like:
bool TargetInstrInfo::isEquivalentTo(GenericOpcode, ExpectedOperand)

LGTM with small nits.

Disclaimer: I haven't never looked at how this pass actually works so take my comments with a grain of salt!

High level comment to help me dive into your changes.
See inlined.

Do you have any comments on the regbankselect part?

Hi Krzysztof,

FWIW, LGTM now.

Hi Krzysztof,

Hi Reid,

Hi Konstantin,

The %1 assignment is dead but not marked as such. I'm not 100% sure whether it is allowed to omit dead flags.

Any chance you could add a test case?

Hi Geoff,

FYI, I've revert the original commit in r325421.

Hi Aditya,