Page MenuHomePhabricator

Fix for PR20059 (instcombine reorders shufflevector after instruction that may trap)
ClosedPublic

Authored by spatel on Jul 8 2014, 12:25 PM.

Details

Summary

In PR20059 ( http://llvm.org/pr20059 ), instcombine eliminates shuffles that are necessary before performing an operation that can trap (srem).

This patch calls isSafeToSpeculativelyExecute() and bails out of the optimization in SimplifyVectorOp() if needed.

I'm not sure if this 'reordering of shuffles' optimization should also be disallowed for all vector FP ops (any of those can cause an exception?), but since there's an existing test case in test/Transforms/InstCombine/vec_shuffle.ll that will have to be removed if we want to change that behavior, I'll post it as a separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 11167.Jul 8 2014, 12:25 PM
spatel retitled this revision from to Fix for PR20059 (instcombine reorders shufflevector after instruction that may trap).
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added a subscriber: Unknown Object (MLST).

Forgot to add reviewers when creating patch.

aschwaighofer edited edge metadata.Jul 9 2014, 9:10 AM

I believe the optimization is safe for floating values that cannot trap.

undef = fadd undef, %x

but it does not trap. So the optimization of reordering the shuffle should be safe.

However:

fdiv %x, undef

can trap because undef can be assumed to be any value including zero. And fdiv traps on zero. So the optimization is not safe.

What does isSafeToSpeculativelyExecute() return for fdiv? It seems to be an omission (bug) that it returns true? (Note, I have not verified this claim but the switch in this function does not contain fdiv and the default case returns true)

At least LICM seems like it could go wrong with conditionally executed fdiv (with loop invariant operands):

bool LICM::isSafeToExecuteUnconditionally(Instruction &Inst) {

// If it is not a trapping instruction, it is always safe to hoist.
if (isSafeToSpeculativelyExecute(&Inst))
  return true;

Actually, now I am not sure myself - does an floating point div trap on zero? :D

We probably get NaN or some infinity - forget what I said.

Your patch LGTM.

spatel closed this revision.Jul 9 2014, 9:43 AM
spatel updated this revision to Diff 11205.

Closed by commit rL212629 (authored by @spatel).

spatel added a comment.Jul 9 2014, 9:52 AM

Thanks, Arnold.

I've committed the fix for int ops, but I'm still doubtful that this is safe for FP (any FP)...because any FP op can cause an exception, right? We don't normally have exceptions enabled for FP div-by-zero, denorms, underflow, etc (especially for vector ops), but a system or user can certainly enable those manually. If that is true and one of the unknown operands in the vector is bad in some way, then we'll generate an exception that wouldn't have occurred without this optimization. I suppose I can generate a C test case to prove this by twiddling the FP exception bits.

hfinkel edited edge metadata.Jul 9 2014, 9:57 AM

"but a system or user can certainly enable those manually" -- No, Clang/LLVM specifically do not support this! In the context of C, this means that we don't support "#pragma STDC FENV_ACCESS on". It is not well defined for the user to fool with the fp environment without informing the compiler (in C, that pragma is the standard mechanism), and if the user tries, they'll receive an error.