Emit assume(false) as store i1 true, ptr poison, align 1

[dianqk]

Store to poison is the canonical form for non-terminator unreachable.

r? ghost

[dianqk]

Also see: llvm/llvm-project#96639 (comment).

[scottmcm]

request: can you please include a comment here linking to something in LLVM about why this is worth doing? Otherwise someone like me would probably come along later and go "well, llvm.assume isn't a terminator so we can just emit that all the time instead" and try to "clean it up".

(Ideally there'd be something in https://llvm.org/docs/ with a bunch of these preferred forms that we could just link to, but no need to block this PR on that.)

[dianqk]

It doesn't look like this is documented. I'll find it or add it.

[scottmcm]

For this PR if it ends up just being to a link to a comment from nikic or something that's fine -- wouldn't be the first time we have one of those in cg_llvm :P

But if you were able to get an official place in LLVM with these kinds of things, that'd be wonderful. I'd love to have a place to reference for all the "don't emit the unsigned checked math intrinsics directly" and such advice!

[dianqk]

request: can you please include a comment here linking to something in LLVM about why this is worth doing? Otherwise someone like me would probably come along later and go "well, llvm.assume isn't a terminator so we can just emit that all the time instead" and try to "clean it up".

(Ideally there'd be something in https://llvm.org/docs/ with a bunch of these preferred forms that we could just link to, but no need to block this PR on that.)

See llvm/llvm-project#98910.

[dianqk]

For this PR if it ends up just being to a link to a comment from nikic or something that's fine -- wouldn't be the first time we have one of those in cg_llvm :P

But if you were able to get an official place in LLVM with these kinds of things, that'd be wonderful. I'd love to have a place to reference for all the "don't emit the unsigned checked math intrinsics directly" and such advice!

I'm guessing this is what you want? https://releases.llvm.org/18.1.0/docs/Frontend/PerformanceTips.html#id10

Avoid using arithmetic intrinsics unless you are required by your source language specification to emit a particular code sequence. The optimizer is quite good at reasoning about general control flow and arithmetic, it is not anywhere near as strong at reasoning about the various intrinsics. If profitable for code generation purposes, the optimizer will likely form the intrinsics itself late in the optimization pipeline. It is very rarely profitable to emit these directly in the language frontend. This item explicitly includes the use of the overflow intrinsics.

[dianqk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit ecd7d6e with merge 9b4768a...

[bors]

☀️ Try build successful - checks-actions
Build commit: 9b4768a (9b4768aca535050097ab0023ada6e7f124395fea)

[rust-timer]

Finished benchmarking commit (9b4768a): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 701.292s -> 699.021s (-0.32%)
Artifact size: 328.63 MiB -> 328.71 MiB (0.02%)

[dianqk]

for reference: https://rust.godbolt.org/z/813oEoxso

[dianqk]

This basically only occurs when ubchecks is enabled.

[scottmcm]

@dianqk Given that instcombine in llvm will convert this (llvm/llvm-project#96639 (comment)) and perf doesn't show any improvements here, can you elaborate on why we'd want to do this? Just emitting the assume is more obvious on the rustc side, IMHO.

If this improved instruction counts or binary size or reduced the number of BBs or something then I'd totally get it, but I'm not sure what the value is currently.

@rustbot author

[dianqk]

@dianqk Given that instcombine in llvm will convert this (llvm/llvm-project#96639 (comment)) and perf doesn't show any improvements here, can you elaborate on why we'd want to do this? Just emitting the assume is more obvious on the rustc side, IMHO.

If this improved instruction counts or binary size or reduced the number of BBs or something then I'd totally get it, but I'm not sure what the value is currently.

@rustbot author

Under ub_checks we can shorten the compile time a bit. Other scenarios don't usually go here. It may make more sense to remove assume when other identical facts exist.

[dianqk]

For example, in the case of llvm/llvm-project#98799, we don't need assume to indicate that the variable is not a null pointer; the load instruction itself already implies this fact.