[RISCV] Filter out instructions which contain unsafe things when outlining

[• pcwang-thead]

Summary

Like FrameIndex and others.

[luismarques]

At first glance this seemed to make sense. The other targets also exclude these operand types, so I guess it really does :)

PS: It's not clear to me what explains some of the remaining differences vs other targets. It would be good to, one of these days, look into harmonizing the accidental differences and documenting the justifiable ones.

[jrtc27]

The commit message says JumpTableIndex but isJTI is already checked?

[jrtc27]

And can we please have regression tests for these cases?

[• pcwang-thead]

In D122635#3437566, @jrtc27 wrote:

The commit message says JumpTableIndex but isJTI is already checked?

My bad. This patch was based on LLVM 13.x and it seems that isJTI was added in D109436. I noticed the conflict when rebasing but I forgot to change the commit message.

[• pcwang-thead]

In D122635#3437559, @luismarques wrote:

At first glance this seemed to make sense. The other targets also exclude these operand types, so I guess it really does :)

PS: It's not clear to me what explains some of the remaining differences vs other targets. It would be good to, one of these days, look into harmonizing the accidental differences and documenting the justifiable ones.

I have taken a look at these differences, most of them are target-dependent and I think we don't need these.

we have tested outlining behaviors on llvm-test-suite, spec2006 and many other benchmarks, and haven't found any errors yet.

But I have no doubt that we may find some potential bugs in the future after enabling default outlining and larger code base's being compiled.

[• pcwang-thead]

In D122635#3437567, @jrtc27 wrote:

And can we please have regression tests for these cases?

I am sorry I haven't found any related tests in other targets and it is really hard to construct one...
I think it makes sense just as other targets do. :)

[• pcwang-thead]

Rebase and update commit message.

[luismarques]

In D122635#3438400, @pcwang-thead wrote:

I am sorry I haven't found any related tests in other targets and it is really hard to construct one...

You might want to go for a MIR test. Let me know if you need help writing one.

[• pcwang-thead]

In D122635#3441085, @luismarques wrote:

In D122635#3438400, @pcwang-thead wrote:

I am sorry I haven't found any related tests in other targets and it is really hard to construct one...

You might want to go for a MIR test. Let me know if you need help writing one.

Thanks!

I have written several MIR tests but found that we don't need to filter out these cases.

• For MO.isCFIIndex(), we have handled CFIs before, so there is no need to do it again.
• For MO.isFI(), because Machine Outliner runs after Prologue/Epilogue Insertion & Frame Finalization, so there is no FrameIndex any more(If I understand correctly).
• For MO.isTargetIndex(), RISCV has no implemented target index, so it is unnecessary. ARM/AArch64/X86 have no target index too, which is weird.

I don't know why other targets exclude these operand types, but I may abandon this patch if it is actually meaningless.

[• pcwang-thead]

Add a FrameIndex releated test.

[• pcwang-thead]

Update test.

[luismarques]

Is this patch still relevant?

[• pcwang-thead]

In D122635#3461473, @luismarques wrote:

Is this patch still relevant?

As what I said before:

I have written several MIR tests but found that we don't need to filter out these cases.

• For MO.isCFIIndex(), we have handled CFIs before, so there is no need to do it again.
• For MO.isFI(), because Machine Outliner runs after Prologue/Epilogue Insertion & Frame Finalization, so there is no FrameIndex any more(If I understand correctly).
• For MO.isTargetIndex(), RISCV has no implemented target index, so it is unnecessary. ARM/AArch64/X86 have no target index too, which is weird.

I don't know why other targets exclude these operand types, but I may abandon this patch if it is actually meaningless.

I think this patch makes no difference to outlining, but it is no harm to land since other targets do the same things.

[luismarques]

In D122635#3461643, @pcwang-thead wrote:

I think this patch makes no difference to outlining, but it is no harm to land since other targets do the same things.

@asb @jrtc27 any opinions on this?

[• pcwang-thead]

D125072 does the favor.

[duck-37]

In D122635#3495850, @pcwang-thead wrote:

D125072 does the favor.

I didn't realize someone else was looking at this at the same time, thanks for bringing this to my attention.

I agree that there are a lot of weird cases in a bunch of targets here -- CFI instructions were checked previously, TargetIndex instructions are used in none of the architectures that check them, etc.

FrameIndex instructions may be important for unwinding, though I don't know that with certainty. EDIT: No, they're removed well before machine outlining is done, as stated before.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

RISCVInstrInfo::getOutliningType(MachineBasicBlock::iterator &MBBI,
   // Don't allow modifying the X5 register which we use for return addresses for
   // these outlined functions.
   if (MI.modifiesRegister(RISCV::X5, TRI) ||
       MI.getDesc().hasImplicitDefOfPhysReg(RISCV::X5))
     return outliner::InstrType::Illegal;
 
   // Make sure the operands don't reference something unsafe.
   for (const auto &MO : MI.operands())
-    if (MO.isMBB() || MO.isBlockAddress() || MO.isCPI() || MO.isJTI())
+    if (MO.isMBB() || MO.isBlockAddress() || MO.isCPI() || MO.isJTI() ||
+        MO.isCFIIndex() || MO.isFI() || MO.isTargetIndex())
       return outliner::InstrType::Illegal;
 
   // Don't allow instructions which won't be materialized to impact outlining
   // analysis.
   if (MI.isMetaInstruction())
     return outliner::InstrType::Invisible;
 
   return outliner::InstrType::Legal;

llvm/test/CodeGen/RISCV/machine-outliner-frame-index.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=riscv32 -x mir -run-pass=machine-outliner -simplify-mir -verify-machineinstrs < %s \
+# RUN: | FileCheck -check-prefix=RV32I-MO %s
+# RUN: llc -march=riscv64 -x mir -run-pass=machine-outliner -simplify-mir -verify-machineinstrs < %s \
+# RUN: | FileCheck -check-prefix=RV64I-MO %s
+
+# Ensure that we won't outline candidates which contains frame indexes.
+
+--- |
+  define void @func1(i32 %a, i32 %b) { ret void }
+
+  define void @func2(i32 %a, i32 %b) { ret void }
+
+  define void @func3(i32 %a, i32 %b) { ret void }
+...
+---
+name:            func1
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, size: 4}
+stack:
+  - { id: 0, type: default, size: 4}
+body:             |
+  bb.0:
+    liveins: $x10, $x11
+    ; RV32I-MO-LABEL: name: func1
+    ; RV32I-MO: liveins: $x10, $x11
+    ; RV32I-MO-NEXT: {{  $}}
+    ; RV32I-MO-NEXT: SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    ; RV32I-MO-NEXT: $x5 = PseudoCALLReg target-flags(riscv-call) @OUTLINED_FUNCTION_0, implicit-def $x5, implicit-def $x10, implicit-def $x11, implicit-def $x12, implicit $x10, implicit $x11
+    ; RV32I-MO-NEXT: SD killed $x10, $x2, 0 :: (store (s32) into %stack.0)
+    ; RV32I-MO-NEXT: PseudoRET
+    ; RV64I-MO-LABEL: name: func1
+    ; RV64I-MO: liveins: $x10, $x11
+    ; RV64I-MO-NEXT: {{  $}}
+    ; RV64I-MO-NEXT: SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    ; RV64I-MO-NEXT: $x5 = PseudoCALLReg target-flags(riscv-call) @OUTLINED_FUNCTION_0, implicit-def $x5, implicit-def $x10, implicit-def $x11, implicit-def $x12, implicit $x10, implicit $x11
+    ; RV64I-MO-NEXT: SD killed $x10, $x2, 0 :: (store (s32) into %stack.0)
+    ; RV64I-MO-NEXT: PseudoRET
+    SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    $x11 = ORI $x11, 1023
+    $x12 = ADDI $x10, 17
+    $x11 = AND $x12, $x11
+    $x10 = SUB $x10, $x11
+    SD killed $x10, $x2, 0 :: (store (s32) into %stack.0)
+    PseudoRET
+...
+---
+name:            func2
+tracksRegLiveness: true
+
+fixedStack:
+  - { id: 0, type: default, size: 4}
+stack:
+  - { id: 0, type: default, size: 4}
+body:             |
+  bb.0:
+    liveins: $x10, $x11
+    ; RV32I-MO-LABEL: name: func2
+    ; RV32I-MO: liveins: $x10, $x11
+    ; RV32I-MO-NEXT: {{  $}}
+    ; RV32I-MO-NEXT: SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    ; RV32I-MO-NEXT: $x5 = PseudoCALLReg target-flags(riscv-call) @OUTLINED_FUNCTION_0, implicit-def $x5, implicit-def $x10, implicit-def $x11, implicit-def $x12, implicit $x10, implicit $x11
+    ; RV32I-MO-NEXT: SD $x11, %stack.0, 0 :: (store (s32) into %stack.0)
+    ; RV32I-MO-NEXT: PseudoRET
+    ; RV64I-MO-LABEL: name: func2
+    ; RV64I-MO: liveins: $x10, $x11
+    ; RV64I-MO-NEXT: {{  $}}
+    ; RV64I-MO-NEXT: SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    ; RV64I-MO-NEXT: $x5 = PseudoCALLReg target-flags(riscv-call) @OUTLINED_FUNCTION_0, implicit-def $x5, implicit-def $x10, implicit-def $x11, implicit-def $x12, implicit $x10, implicit $x11
+    ; RV64I-MO-NEXT: SD $x11, %stack.0, 0 :: (store (s32) into %stack.0)
+    ; RV64I-MO-NEXT: PseudoRET
+    SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    $x11 = ORI $x11, 1023
+    $x12 = ADDI $x10, 17
+    $x11 = AND $x12, $x11
+    $x10 = SUB $x10, $x11
+    SD $x11, %stack.0, 0 :: (store (s32) into %stack.0)
+    PseudoRET
+...
+---
+name:            func3
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, size: 4}
+stack:
+  - { id: 0, type: default, size: 4}
+body:             |
+  bb.0:
+    liveins: $x10, $x11
+    ; RV32I-MO-LABEL: name: func3
+    ; RV32I-MO: liveins: $x10, $x11
+    ; RV32I-MO-NEXT: {{  $}}
+    ; RV32I-MO-NEXT: SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    ; RV32I-MO-NEXT: $x5 = PseudoCALLReg target-flags(riscv-call) @OUTLINED_FUNCTION_0, implicit-def $x5, implicit-def $x10, implicit-def $x11, implicit-def $x12, implicit $x10, implicit $x11
+    ; RV32I-MO-NEXT: SD $x11, %stack.0, 0 :: (store (s32) into %stack.0)
+    ; RV32I-MO-NEXT: PseudoRET
+    ; RV64I-MO-LABEL: name: func3
+    ; RV64I-MO: liveins: $x10, $x11
+    ; RV64I-MO-NEXT: {{  $}}
+    ; RV64I-MO-NEXT: SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    ; RV64I-MO-NEXT: $x5 = PseudoCALLReg target-flags(riscv-call) @OUTLINED_FUNCTION_0, implicit-def $x5, implicit-def $x10, implicit-def $x11, implicit-def $x12, implicit $x10, implicit $x11
+    ; RV64I-MO-NEXT: SD $x11, %stack.0, 0 :: (store (s32) into %stack.0)
+    ; RV64I-MO-NEXT: PseudoRET
+    SD $x10, %fixed-stack.0, 0 :: (store (s32) into %fixed-stack.0)
+    $x11 = ORI $x11, 1023
+    $x12 = ADDI $x10, 17
+    $x11 = AND $x12, $x11
+    $x10 = SUB $x10, $x11
+    SD $x11, %stack.0, 0 :: (store (s32) into %stack.0)
+    PseudoRET