- User Since
- Dec 17 2012, 10:03 AM (291 w, 1 d)
Mon, Jun 18
Either changing the default (trackLiveness == true by default) or printing it is fine.
Jun 15 2018
Are you okay with us solving this just for STATEPOINTs at the moment by essentially just disabling remats for STATEPOINT uses?
May 25 2018
Do you agree we could drop this patch?
May 24 2018
With the test case fixed
Up to you for the logging. We can keep it.
May 23 2018
Doesn't the target already do this? At least there's a CoveredBySubRegs flag in the Register .td class ...
May 22 2018
May 18 2018
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.
May 15 2018
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
May 11 2018
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
May 7 2018
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.
May 4 2018
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.
May 3 2018
Couple of nits on the tests.
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?
Apr 27 2018
Apr 24 2018
Apr 23 2018
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.
Mar 22 2018
Do you have any comments on the regbankselect part?
Mar 19 2018
Mar 14 2018
FWIW, LGTM now.
Mar 5 2018
Feb 27 2018
The %1 assignment is dead but not marked as such. I'm not 100% sure whether it is allowed to omit dead flags.
Feb 26 2018
Any chance you could add a test case?
Feb 21 2018
Feb 16 2018
FYI, I've revert the original commit in r325421.
Thanks all for the great feedbacks!
Feb 13 2018
(though it may require the interface to be changed slightly since we'll need the opcode)
Feb 12 2018
Feb 9 2018
Feb 8 2018
BTW, while writing the RFC, I realized that we could potentially generated incorrect code if we were silently downgrade a post-r317488 bitcode with a pre-r317488 compiler. (I.e., running fast math optimizations whereas we only wanted reassoc)
I think we have to create a new version of the IR since the bits changed meaning (we can't just flip 'on' new bits).
For the review itself, I leave this to @aprantl. I was just providing insights on the shrink-wrapping question :).
Feb 6 2018
Feb 5 2018
Feb 2 2018
One question below.
LGTM, coordinate with the x86 changes and you should be good to go!
Feb 1 2018
While I was here, I notice that selectCopy was not using getRegClassForTypeOnBank. Could you double check this is intended?
LGTM with the nitpicks mentioned by Krzysztof.
Jan 31 2018
Jan 30 2018
Just two remarks:
- So far shrink-wrapping was solely an analysis pass, i.e., it didn't modify the input code
- Ideally I'd like we postpone creating new blocks until we decided where we are going to insert the code, otherwise we'll end up with blocks to clean-up later on
Ah makes sense.
Let's leave the patch as is.
Jan 29 2018
Alright, LGTM then.
Please also patches the two other places I mentioned assuming you can have a test case for them.
Got a chat with Roman off-line and turns out the problem I mentioned is worked around by everything single target, so indeed no test cases!
Couple of nits on the test case.