- User Since
- Jan 28 2016, 8:30 AM (89 w, 5 d)
Tue, Sep 19
Sep 12 2017
SIInstrInfo::doMemOpsHaveSameBasePtr looks generic enough to be default indeed but should we commit this first and then make doMemOpsHaveSameBasePtr default so we could rollback to this one in case of severe regressions? LGTM by the way.
Sep 8 2017
Wow! I wish I had this before! LGTM.
Sep 6 2017
LGTM. However it looks like we should fix TD to follow single naming convention.
Sep 4 2017
Sep 1 2017
Ok, unmangled part looks different indeed. If the issue with pre-link checking is solved this patch is ok with me.
Splitting AMDGPULibFunc in two classes looks a huge overkill. How about modifying AMDGPULibFunc::parse so it could accept unmangled names and just return an enum id for the function (using some fast lookup approach)? Type info for such functions can be left unpopulated and supposed to be handled by the client (as in fold_read_write_pipe).
Aug 28 2017
Aug 11 2017
Jul 21 2017
The implementation of this approach looks good to me. The only question is which way to go to implement v3 vector.
Jul 20 2017
Jun 27 2017
Jun 20 2017
LGTM, with nit.
May 26 2017
May 23 2017
May 22 2017
May 19 2017
fixed as per comments
May 18 2017
I agree if this is a bugfix.
I inserted MachineInstrRegs between reset and recede, all functions are in old places. I should move MachineInstrRegs higher
May 17 2017
May 16 2017
I'm a bit confused with all of these advance... but lets submit and improve this later.
May 15 2017
You can do the reset with move - this would not require copy.
I really don't understand it all. Why not just have reset(MachineInstr *, LiveRegSet ) and do a reset on the next BB with liveset from previous BB? It looks like your code does it. Another question: your reset does skip debugs, but the caller doesn't know about it and can supply unskipped iterator to the next advance.
This looks a bit as using carkeys to open a bottle. Should we have single instruction RP diff returning function without changing any state?
If I don't mistake downward tracker cannot work on arbitrary instruction order, so let's change it's interface so that it tracks current instruction and move it, likewise like llvm standard tracker, that is reset(MI), advance()
May 12 2017
May 11 2017
Apr 26 2017
Apr 25 2017
Apr 21 2017
Looks very good now! Thanks!
Apr 20 2017
Thank you for doing this! I really need it.
Apr 13 2017
Apr 12 2017
For the second assert it may be the case for iterator invalidation, though I haven't checked.
Apr 10 2017
Apr 7 2017
Apr 6 2017
The following tests assert with this (and predecessors) patches with SISched turned on by default:
Apr 5 2017
Apr 4 2017
First part of comments related only to C++ issues
Mar 31 2017
If SU(i) uses register produced by SI(j):
I may miss something, but it looks that you can build data edges when building a superdag consisting of blocks. Incoming data edges would be liveins, outcoming - liveouts.
In general, I think moving instructions just to use standard RP tracker to discover liveins/liveouts isn't a good idea. It isn't only slow but doesn't look reliable too. Why not discover these sets using DAG directly?
I didn't debugged it and I don't know why you decided so.
I ran lit tests with sished with ShouldTrackLaneMasks=true enabled by default with this patch, the following tests asserted:
Mar 29 2017
Mar 28 2017
Axel, thanks for the update.
This looks elegant, mine was longer :), thanks!
Ok, I understood SIScheduleBlockCreator::scheduleInsideBlocks() moves instructions to actually get LiveIn and LiveOut set for a block, but this is rather heavy. Have you thought about getting those using DAG directly, not regpressure tracker? By the common sence the dependencies between blocks correspond to that liveness info. There is a problem however: LiveIn and LiveOut dependencies aren't modelled for boundary SUs. I have local patch that build such dependencies - scheduling region LiveIns edges comes from EntrySU, LiveOut - to ExitSU. Another problem - dependency edges doesn't have lanemask, need to think how to deal with this.
Why SIScheduleBlockCreator::scheduleInsideBlocks() actually move instructions? Why it isn't done on the final scheduling?