This is an archive of the discontinued LLVM Phabricator instance.

SpeculativeExecution: fixed ingoring free execution
ClosedPublic

Authored by dfukalov on Feb 19 2020, 3:31 AM.

Details

Summary

After updating cost model in AMDGPU target (47a5c36b37f0) the pass started to
ignore some BBs since they got all instructions estimated as free.

Diff Detail

Event Timeline

dfukalov created this revision.Feb 19 2020, 3:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2020, 3:31 AM
nhaehnle accepted this revision.Feb 19 2020, 3:56 AM

LGTM, though you may want to wait a day or two to allow others to chime in.

This revision is now accepted and ready to land.Feb 19 2020, 3:56 AM
arsenm added inline comments.Feb 19 2020, 8:40 AM
llvm/test/CodeGen/AMDGPU/speculative-execution-freecasts.ll
13

Would probably be better to have real values, since someday something might conclude any undef operation is free or something

dfukalov updated this revision to Diff 245612.Feb 20 2020, 3:43 AM

Test updated as requested

dfukalov marked an inline comment as done.Feb 20 2020, 3:44 AM
This revision was automatically updated to reflect the committed changes.

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

The pass isn't supposed to even run for CPU targets

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

The pass isn't supposed to even run for CPU targets

I think this pass is also running for non-GPU targets. I'm seeing this pass run on x86_64, aarch64, and riscv64 when building a toolchain for those targets.

https://github.com/llvm/llvm-project/blob/a4cde9ad7b6f1a4cfef228f6cf2fc4911bf24c77/llvm/lib/Passes/PassBuilder.cpp#L436 seems to add it to the new PM default function pipeline that I think runs as long as optimizations are available.

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

The pass isn't supposed to even run for CPU targets

I think this pass is also running for non-GPU targets. I'm seeing this pass run on x86_64, aarch64, and riscv64 when building a toolchain for those targets.

https://github.com/llvm/llvm-project/blob/a4cde9ad7b6f1a4cfef228f6cf2fc4911bf24c77/llvm/lib/Passes/PassBuilder.cpp#L436 seems to add it to the new PM default function pipeline that I think runs as long as optimizations are available.

That doesn't match what the comment says, or the old PM does (which does createSpeculativeExecutionIfHasBranchDivergencePassz)

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

The pass isn't supposed to even run for CPU targets

I think this pass is also running for non-GPU targets. I'm seeing this pass run on x86_64, aarch64, and riscv64 when building a toolchain for those targets.

https://github.com/llvm/llvm-project/blob/a4cde9ad7b6f1a4cfef228f6cf2fc4911bf24c77/llvm/lib/Passes/PassBuilder.cpp#L436 seems to add it to the new PM default function pipeline that I think runs as long as optimizations are available.

That doesn't match what the comment says, or the old PM does (which does createSpeculativeExecutionIfHasBranchDivergencePassz)

@chandlerc It seems like you added the pass to the new PM pipelne in https://reviews.llvm.org/rGe3f5064b7235 with the main difference being OnlyIfDivergentTarget false whereas OnlyIfDivergentTarget is true in the old PM with createSpeculativeExecutionIfHasBranchDivergencePass. Is this intentional?

Hi!

I just wrote a PR about a debug-info fault that starts occuring with this patch:
https://bugs.llvm.org/show_bug.cgi?id=46267

I've just got the bug, will take a look