Page MenuHomePhabricator

[NewPM] Run the Speculative Execution Pass only if the target has divergent branches
AbandonedPublic

Authored by leonardchan on Mar 18 2020, 3:06 PM.

Details

Summary

The SpeculativeExecutionPass was running under the new PM for non-GPU targets. It appears that the pass would run regardless as long as speed optimizations were available. Run the Speculative Execution Pass only if the target has divergent branches.

Diff Detail

Event Timeline

leonardchan created this revision.Mar 18 2020, 3:06 PM

Commit message should say only if?

leonardchan retitled this revision from [NewPM] Run the Speculative Execution Pass if the target has divergent branches to [NewPM] Run the Speculative Execution Pass only if the target has divergent branches.Mar 30 2020, 3:57 PM

Commit message should say only if?

Updated

arsenm added inline comments.Apr 1 2020, 3:34 PM
clang/test/CodeGen/thinlto-distributed-newpm.ll
110 ↗(On Diff #251182)

The pass name shouldn't change here?

llvm/lib/Passes/PassBuilder.cpp
443

I don't expect this to be a separate pass, just added based on a divergent target

leonardchan edited the summary of this revision. (Show Details)
leonardchan marked an inline comment as done.
leonardchan added inline comments.
llvm/lib/Passes/PassBuilder.cpp
443

Updated such that the current pass is used, although it's a bit more difficult to test since the same pass name will appear when dumping the passes.

arsenm added inline comments.Apr 14 2020, 7:39 AM
llvm/include/llvm/Transforms/Scalar/SpeculativeExecution.h
97 ↗(On Diff #255474)

Still define a second pass

llvm/lib/Passes/PassBuilder.cpp
443

There's a check for divergent branches in the builder so you don't need to run the pass at all

leonardchan marked an inline comment as done.
leonardchan edited the summary of this revision. (Show Details)

This seems like it covers a different case than D82735?

arsenm requested changes to this revision.Jul 7 2020, 11:48 AM
This revision now requires changes to proceed.Jul 7 2020, 11:48 AM
leonardchan abandoned this revision.Jul 8 2020, 12:31 PM

This seems like it covers a different case than D82735?

I haven't rebased in a while, but D82735 covers this. Abandoning since this is addressed in that patch.