This is an archive of the discontinued LLVM Phabricator instance.

[Local] Flag to decide if a trap should be placed before unreachable
AbandonedPublic

Authored by jdoerfert on Jul 19 2021, 12:06 PM.

Details

Summary

Before we used "custom heuristics" to determine if a newly placed
unreachable should be preceded by a trap (llvm.trap) or not. This was
undesirable for multiple reasons, mainly it causes traps where we might
really want to avoid them (GPU targets), and it didn't allow to insert
more traps with proper debug information for debugging. The new scheme
allows the user to overwrite the standard "context decision" and force
or disallow traps instead.

This patch should not change the behavior on it's own but just provide
more options. The plan is to update the default for OpenMP GPU targets.

Diff Detail

Event Timeline

jdoerfert created this revision.Jul 19 2021, 12:06 PM
jdoerfert requested review of this revision.Jul 19 2021, 12:06 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: bbn, sstefan1. · View Herald Transcript

I think TrapBeforeUnreachableChoice should be a TTI hook, overrided by GPU targets.
I think shouldUseTrapBeforeUnreachable() should be sunk into changeToUnreachable(), and UseLLVMTrap renamed into PreferToUseLLVMTrap, with an appropriate documentation opdate.

I think TrapBeforeUnreachableChoice should be a TTI hook, overrided by GPU targets.

While I don't mind that in general, I figured "always" is a nice debugging option and should be exposed. Not sure what a nice way would be to get both things.

I think shouldUseTrapBeforeUnreachable() should be sunk into changeToUnreachable(), and UseLLVMTrap renamed into PreferToUseLLVMTrap, with an appropriate documentation opdate.

I can do that.

I think TrapBeforeUnreachableChoice should be a TTI hook, overrided by GPU targets.

While I don't mind that in general, I figured "always" is a nice debugging option and should be exposed. Not sure what a nice way would be to get both things.

That's another reason why i'm not really in favor of this approach.
Even ignoring the subset of users that will want this for hardening reasons,
i'd really expect that always really means before each unreachable,
but clearly it won't appear before the already-unreachable terminators.

I think shouldUseTrapBeforeUnreachable() should be sunk into changeToUnreachable(), and UseLLVMTrap renamed into PreferToUseLLVMTrap, with an appropriate documentation opdate.

I can do that.

Address comment and make it PreferToUseLLVMTrap

llvm/lib/Transforms/Utils/Local.cpp
131

s/awlays/always

efriedma added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
2351

This (and the corresponding handling for calls, below) is the only place where we're passing PreferToUseLLVMTrap=true, right?

It looks like there was an attempt to remove the trap generation back in 2016, but it got reverted. Revert said it "it seems to break UBSan". (rGe14e7bc4b889dfaffb7180d176a03311df2d4ae6). Maybe worth attempting again; we really shouldn't need to insert a trap after non-volatile store to null.

(Note this has nothing to do with the backend converting an "unreachable" to a trap instruction. Some backends do this for various reasons. For example, on Windows, it's necessary to generate correct EH tables in some cases, I think.)

jdoerfert added inline comments.Jul 19 2021, 1:05 PM
llvm/lib/Transforms/Utils/Local.cpp
2351

This (and the corresponding handling for calls, below) is the only place where we're passing PreferToUseLLVMTrap=true, right?

It seems that way. I'm happy to remove the trap stuff and we wait and see if UBSan is unhappy or not.

lebedev.ri added inline comments.Jul 19 2021, 1:11 PM
llvm/lib/Transforms/Utils/Local.cpp
2351

SGTM

jdoerfert abandoned this revision.Jul 19 2021, 2:15 PM

Replaced by D106308