[llvm] Check for all frame instructions in finalize isel. (PR #85945)

Jonas Paulsson via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 25 11:15:52 PDT 2024


JonPsson1 wrote:

I agree it is slightly troublesome that the code in the backend can return any new MBB while making assumptions about which instructions to skip, and also by that have the FinalizeISelPass skip them. This was trivial as long as FinalizeISel only was interested in pseudos with custom insertion. As this changed, the backends assumption also had to change. In this case the SystemZ backend will be ok as long as the frame destroy opcode is also checked.

Maybe EmitInstrWithCustomInserter() should always return an iterator from where FinalizeISel should continue? That way the backend could scan forward a bit and skip things its unaware of while still returning an iterator so that this range is checked by FinalizeISel still.

Alternatively, maybe a comment something like this:

```
diff --git a/llvm/lib/CodeGen/FinalizeISel.cpp b/llvm/lib/CodeGen/FinalizeISel.cpp
index bf967eac22f1..9b920f6eaf82 100644
--- a/llvm/lib/CodeGen/FinalizeISel.cpp
+++ b/llvm/lib/CodeGen/FinalizeISel.cpp
@@ -67,6 +67,13 @@ bool FinalizeISel::runOnMachineFunction(MachineFunction &MF) {
         Changed = true;
         MachineBasicBlock *NewMBB = TLI->EmitInstrWithCustomInserter(MI, MBB);
         // The expansion may involve new basic blocks.
+
+        // NB! The target may skip past instructions while assuming they are
+        // not of interest to this pass, for example when SystemZ combines
+        // multiple selects into a single branch sequence (emitSelect). So if
+        // some new type of instruction becomes important here, it must be
+        // accompanied with some kind of assertion to make sure the backend
+        // does not mess this up.
         if (NewMBB != MBB) {

```
@arsenm @uweigand 

https://github.com/llvm/llvm-project/pull/85945


More information about the llvm-commits mailing list