[FinalizelSel] Re-scan the MachineFunction if we insert a new MBB by custom insertion

[jacquesguan]

Current logic may skip some MI and would have wrong adjustsStack flag.

[paperchalice]

Can we just handle missed MIs correctly instead of iterating from begin again?

[jacquesguan]

Would it insert more than 1 MBB? I am not sure how should we set the MF iterator to make it at the first inserted MBB's iteration.

[paperchalice]

I'm not familiar with RISCV, IIUC the old behavior may skip the rest MIs in I if new blocks are inserted after I, and skipped MIs may need expansion. If so, can we check the next BB is the new BB to set iterators correctly?

[jacquesguan]

I change to continue scan current MBB.

[paperchalice]

LLDB and some LoongArch tests failed, but doesn't look relevant to this pull request.

[topperc]

If I'm reading the failing test right, we have a select before a call to memset and a select after the call. We merge them into a single control flow change. Is that really something we should be doing?

We already abort the scan for

    if (SequenceMBBI->hasUnmodeledSideEffects() ||                               
        SequenceMBBI->mayLoadOrStore() ||                                        
        SequenceMBBI->usesCustomInsertionHook())                                 
      break;   

Should call be included? A call could contain loads, stores, or unmodelled side effects.

Adding @asb as well

[topperc]

I dug into this some more. The emitInstrWithCustomInsert interface dates back to a time when it was called from InstrEmitter.cpp as MachineInstrs were created from SelectionDAG. There was nothing later in the block. The MBB returned was used to reset the position for where future instructions should be inserted.

Maybe a better interface today would return the iterator where to resume scanning.

[jacquesguan]

If I'm reading the failing test right, we have a select before a call to memset and a select after the call. We merge them into a single control flow change. Is that really something we should be doing?

We already abort the scan for

    if (SequenceMBBI->hasUnmodeledSideEffects() ||                               
        SequenceMBBI->mayLoadOrStore() ||                                        
        SequenceMBBI->usesCustomInsertionHook())                                 
      break;   

Should call be included? A call could contain loads, stores, or unmodelled side effects.

Adding @asb as well

Yes, change emitSelectPseudo would also resolve the problem, but I think the root reason of this issue is the scanning logic of FinalizeISel.
When we insert one or some MBB, it assumes that the current MBB would be split in the custom insertion and EmitInstrWithCustomInserter should return the last MBB inserted.
The RISCV backend has different behavior, it will continue to scan the current MBB and try merge several select pseudo, so that it would skip some MIs.
I think changing the interface should be a better way.

[jacquesguan]

I dug into this some more. The emitInstrWithCustomInsert interface dates back to a time when it was called from InstrEmitter.cpp as MachineInstrs were created from SelectionDAG. There was nothing later in the block. The MBB returned was used to reset the position for where future instructions should be inserted.

Maybe a better interface today would return the iterator where to resume scanning.

Thanks, I will try this way.

[spaits]

Hi @jacquesguan @topperc . You have mentioned that breaking at calls in emitSelectPseudo would fix the issue, but it is not the root cause of the it. In the current state of things, we are generation wrong code if the assertions are disabled. Could we please at least temporarily do the above mentioned quick fix in emitSelectPseudo so we are not generating wrong code? Would that work @topperc ? If so would you @jacquesguan like to do it or should I create a PR with that?

Thank you.