[PATCH] D106299: [Local] Flag to decide if a trap should be placed before unreachable

Mon Jul 19 13:05:11 PDT 2021

jdoerfert added inline comments.

================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2351
                                    SI->getPointerAddressSpace()))) {
-          changeToUnreachable(SI, true, false, DTU);
+          changeToUnreachable(SI, /* PreferToUseLLVMTrap */ true, false, DTU);
           Changed = true;
----------------
efriedma wrote:
> 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.)
> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106299/new/

https://reviews.llvm.org/D106299