Page MenuHomePhabricator

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

Authored by pcwang-thead on Mar 28 2022, 10:25 PM.

Details

Summary

Like FrameIndex and others.

Diff Detail

Event Timeline

pcwang-thead created this revision.Mar 28 2022, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 10:25 PM
pcwang-thead requested review of this revision.Mar 28 2022, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 10:25 PM
luismarques accepted this revision.Apr 7 2022, 3:51 PM

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.

This revision is now accepted and ready to land.Apr 7 2022, 3:51 PM
jrtc27 added a comment.Apr 7 2022, 3:55 PM

The commit message says JumpTableIndex but isJTI is already checked?

jrtc27 added a comment.Apr 7 2022, 3:55 PM

And can we please have regression tests for these cases?

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.

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.

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. :)

Rebase and update commit message.

pcwang-thead edited the summary of this revision. (Show Details)Apr 8 2022, 2:49 AM

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.

luismarques requested changes to this revision.Apr 9 2022, 9:08 AM
This revision now requires changes to proceed.Apr 9 2022, 9:08 AM

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.

Add a FrameIndex releated test.

Is this patch still relevant?

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.

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 abandoned this revision.May 5 2022, 11:17 PM

D125072 does the favor.

duck-37 added a subscriber: duck-37.EditedMay 6 2022, 7:25 AM

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.