This is an archive of the discontinued LLVM Phabricator instance.

BPF: Implement TTI.IntImmCost() properly
ClosedPublic

Authored by yonghong-song on Feb 10 2021, 1:10 PM.

Details

Summary

This patch implemented TTI.IntImmCost() properly.
Each BPF insn has 32bit immediate space, so for any immediate
which can be represented as 32bit signed int, the cost
is technically free. If an int cannot be presented as
a 32bit signed int, a ld_imm64 instruction is needed
and a TCC_Basic is returned.

This change is motivated when we observed that
several bpf selftests failed with latest llvm trunk, e.g.,

#10/16 strobemeta.o:FAIL
#10/17 strobemeta_nounroll1.o:FAIL
#10/18 strobemeta_nounroll2.o:FAIL
#10/19 strobemeta_subprogs.o:FAIL
#96 snprintf_btf:FAIL

The reason of the failure is due to that
SpeculateAroundPHIsPass did aggressive transformation
which alters control flow for which currently verifer
cannot handle well. In llvm12, SpeculateAroundPHIsPass
is not called.

SpeculateAroundPHIsPass relied on TTI.getIntImmCost()
and TTI.getIntImmCostInst() for profitability
analysis. This patch implemented TTI.getIntImmCost()
properly for BPF backend which also prevented
transformation which caused the above test failures.

Diff Detail

Event Timeline

yonghong-song created this revision.Feb 10 2021, 1:10 PM
yonghong-song requested review of this revision.Feb 10 2021, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 1:10 PM

I get concerned when I see us having to disable optimizations to fix bugs. Is there any kind of long-term plan to make the kernel verifier able to accept what LLVM produces or will we always be chasing after failures like this?

yonghong-song retitled this revision from BPF: disable SpeculateAroundPHIsPass with TTI IntImmCost TCC_Free to BPF: Implement TTI.IntImmCost() properly.
yonghong-song edited the summary of this revision. (Show Details)

Rather than disabling SpeculateAroundPHIsPass, implement TTI.IntImmCost() properly, which also solved our issues.

I get concerned when I see us having to disable optimizations to fix bugs. Is there any kind of long-term plan to make the kernel verifier able to accept what LLVM produces or will we always be chasing after failures like this?

I just made the changes, not to disable optimizations, rather implements proper callbacks, which also fixed the issue.

ast accepted this revision.Feb 10 2021, 8:29 PM

looks great. thanks

This revision is now accepted and ready to land.Feb 10 2021, 8:29 PM
This revision was landed with ongoing or failed builds.Feb 11 2021, 8:35 AM
This revision was automatically updated to reflect the committed changes.