In D143287#4202174, @ManuelJBrito wrote:

Implementing the 128 to 512 casts by filling the rest of the vector with the same definition of a nondeterministic_value is not correct because :

a = freeze poison
v = <a, a>

is not the same as

v = freeze poison

The only solution I'm seeing ,using the shufflevector, is doing the conversion in two steps:

• build a 256 vector with the upper half being undefined( freeze poison)
• build a 512 vector where the lower half is the previous 256 vector and the upper half being undefined

I think this would require two shuffles which is unfortunate.

This would ensure no miscompilations due to multiple uses of the same freeze undef/poison but would probably require some backend work to ensure the pattern is recognized to emit efficient assembly.

LGTM

In D144903#4182498, @efriedma wrote:

I think the point of the hasOneUse check is to avoid a possible miscompile; if a FREEZE has more than one use, all users need to see the same value. So not sure dropping the check is correct in general.

In D145632#4181185, @kritgpt wrote:

In D145632#4180536, @nlopes wrote:

tests are failing. please run all the tests before submitting patches.

I checked these files with ninja check-llvm and all tests passed, but some fail here. Is there other checks I can perform before submitting a patch?

tests failing

tests failing

tests failing

tests are failing. please run all the tests before submitting patches.

Please use poison values as place holders instead of undef values as we're trying to get rid of Undef. Thank you!

This is great, thank you!

In D141386#4069054, @nikic wrote:

In D141386#4048676, @nlopes wrote:

Three failures:

• Transforms/InstCombine/loadstore-metadata.ll
• Transforms/Mem2Reg/preserve-nonnull-load-metadata.ll
• Transforms/SROA/preserve-nonnull.ll

The last two should be fixed by https://github.com/llvm/llvm-project/commit/e6241cbdcbf3cc9beb49460578466e18936ef220.

LGTM, except for the align metadata.

The issue is the function call, not the stores.
Stores are valid as long as they are to local memory (stack or allocated by the function).

Three failures:

• Transforms/InstCombine/loadstore-metadata.ll
• Transforms/Mem2Reg/preserve-nonnull-load-metadata.ll
• Transforms/SROA/preserve-nonnull.ll

In D141386#4047344, @nikic wrote:

In D141386#4044303, @nlopes wrote:

Sounds good, thanks!
I implemented this in Alive2 and I got zero failures in LLVM's test suite. Either it doesn't have many !ranges or we are good.

Shouldn't this test have failed? https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/Mem2Reg/preserve-nonnull-load-metadata.ll Or did you only adjust !range but not !nonnull?

In D141386#4047344, @nikic wrote:

In D141386#4044303, @nlopes wrote:

Sounds good, thanks!
I implemented this in Alive2 and I got zero failures in LLVM's test suite. Either it doesn't have many !ranges or we are good.

Shouldn't this test have failed? https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/Mem2Reg/preserve-nonnull-load-metadata.ll Or did you only adjust !range but not !nonnull?

Sounds good, thanks!
I implemented this in Alive2 and I got zero failures in LLVM's test suite. Either it doesn't have many !ranges or we are good.

LGTM

I must say that this patch is a bit annoying :)
We are trying to remove undef, and you're adding yet another use.
It's true we can't use poison here. The alternative in the no-undef world is 'freeze poison'. Or just leave the zero there.
Is this change important? If not, I would prefer to not do it, as we'll have to revert it when removing undef (hopefully later this year).
Thanks!

LGTM, thanks!

LGTM!

In D134410#3983684, @nikic wrote:

The other problem I see here is that we still need to do something about the memcpy -> load/store fold. As soon as we have poison from uninit values (either directly or via !uninit_is_poison) this will start causing miscompiles very quickly. The only way to do this right now is again with an <N x i8> vector load/store, which still optimizes terribly. This needs either load+store of integer to preserve poison, or again some form of byte type.

Something I've only recently realized is that we also run into this problem when inserting spurious load/store pairs, e.g. as done by LICM scalar promotion. If we're promoting say an i32 value to scalar that previously used a conditional store, then promotion will now introduce an unconditional load and store, which will spread poison from individual bytes. So that means that technically scalar promotion (and SimplifyCFG store speculation) also need to do some special dance to preserve poison. And without preservation of poison across load/store/phi in plain ints, this is going to be a bad optimization outcome either way: We'd have to use either a vector of i8 or a byte type for the inserted phi nodes and only cast to integer and back when manipulating the value, which would probably defeat the optimization. At that point probably best to freeze the first load (which again needs a freezing load).

LGTM, thank you!

In D131628#4000637, @nikic wrote:

@nlopes With D137360 up, do you still have concerns about this specification?

In rG4446f71ce392a13e64299d4efe9d7c2a2b768c6c#1155386, @spatel wrote:

In rG4446f71ce392a13e64299d4efe9d7c2a2b768c6c#1155371, @nlopes wrote:

This because once you bitcast the vector to have fewer elements, poison propagates to neighbors. I think we can't do this part of the optimization.

Interesting - so we can't ever bitcast an arbitrary vector to fewer (larger) elements. But the tests where we are inserting into a full undef/poison base vector are still ok? (because that guarantees there's no potential to spill poison over to an element that wasn't already poison)

In D138766#3996152, @lebedev.ri wrote:

Thank you for looking into it!

In D138766#3996126, @nlopes wrote:

In D138766#3995966, @lebedev.ri wrote:

In D138766#3995896, @nlopes wrote:

FWIW, I've discovered today that GVN does a similar optimization (but without the freeze..).
See here (scroll to the bottom): https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=aed14c64378404c9&test=Transforms%2FPhaseOrdering%2FX86%2Fvec-load-combine.ll

That seems to be with constant indexes, though?

True.
So it could use a simple extractelement rather than bit masking.

Define "could"? Define "simple"?
I've looked at alternative lowerings (shufflevector or chain of extractelement's),
and they all result in worse codegen. We can not use a single `extractelement,
because the byte offset may not be a multiple of the element size.
The shift is the optimal lowering here, any alternative chosen lowering
would need to be canonicalized into it, and and which point why bother?

In D138766#3995966, @lebedev.ri wrote:

In D138766#3995896, @nlopes wrote:

FWIW, I've discovered today that GVN does a similar optimization (but without the freeze..).
See here (scroll to the bottom): https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=aed14c64378404c9&test=Transforms%2FPhaseOrdering%2FX86%2Fvec-load-combine.ll

That seems to be with constant indexes, though?

Alive complains about this test case:

define <8 x i16> @insert_10_v8i16(i32 %x, <8 x i16> %v) {
%0:
  %hi32 = lshr i32 %x, 16
  %hi16 = trunc i32 %hi32 to i16
  %lo16 = trunc i32 %x to i16
  %ins0 = insertelement <8 x i16> %v, i16 %lo16, i64 1
  %ins1 = insertelement <8 x i16> %ins0, i16 %hi16, i64 0
  ret <8 x i16> %ins1
}
=>
define <8 x i16> @insert_10_v8i16(i32 %x, <8 x i16> %v) {
%0:
  %1 = bitcast <8 x i16> %v to <4 x i32>
  %2 = insertelement <4 x i32> %1, i32 %x, i64 0
  %ins1 = bitcast <4 x i32> %2 to <8 x i16>
  ret <8 x i16> %ins1
}
Transformation doesn't verify! (unsound)
ERROR: Target is more poisonous than source

FWIW, I've discovered today that GVN does a similar optimization (but without the freeze..).
See here (scroll to the bottom): https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=aed14c64378404c9&test=Transforms%2FPhaseOrdering%2FX86%2Fvec-load-combine.ll

LGTM

In D139785#3986910, @RalfJung wrote:

The actual issue is that X > 0.0 is false for negative NaNs

LGTM

LGTM

Thanks.
unspecified is not a term used in LLVM IR semantics. I would write, for example, "a non-deterministic value (equivalent to freeze poison)".

In D139312#3970743, @foad wrote:

Seems reasonable. Can we get alive2 proofs for floating point things like this yet @nlopes?

LGTM

Manuel: there are quite a few test cases failing in the build bots: https://lab.llvm.org/buildbot/#/builders/16/builds/38352

LGTM

In D136320#3930388, @Orlando wrote:

In D136320#3870269, @nlopes wrote:

Please avoid using UndefValue::get as much as possible as we are trying to get rid of undef. Please use PoisonValue whenever possible.
Thank you!

Hi @nlopes. This patch is part of the series in which we agreed this could be addressed after landing the stack: https://reviews.llvm.org/D133293#inline-1286892. That is the next item on my TODO list and I plan to make the change for all debug info modes (rather than just this new one). Is that still okay with you?

In D131628#3900066, @mysterymath wrote:

@nlopes, can we create a separate issue for finding the right semantics, and move forward by making this change describe as accurately as possible the current state of affairs?
Given that the original patch has been in LLVM for a long time, spanning many versions, it seems like we'd want to rectify the undocumentedness of the attribute ASAP, then work towards changing its semantics afterwards.

In D131628#3899670, @mysterymath wrote:

In D131628#3898605, @nlopes wrote:

So, dropping nocallback during linking seems the way to go (and adjust the docs accordingly). Once you link, you can do inter-proc analysis and recover that info if needed anyway.

This would make Clang the only implementation of __attribute__((leaf)) to do so.
It's also not generally possible to recover the nocallback information about functions external to the LTO unit, which is the only thing __attribute__((leaf)) is actually good for in full LTO.

In D134410#3898563, @nikic wrote:

In D134410#3895253, @nlopes wrote:

In D134410#3895077, @nikic wrote:

In D134410#3894995, @nlopes wrote:

We wanted this patch to make us switch uninitialized loads to poison at will, since they become UB. In practice, this helps us fixing bugs in SROA and etc without perf degradation.

Can you elaborate on this? I don't see how this is necessary for switching uninitialized loads to poison.

It's not mandatory, it's a simple way of achieving it as !noundef already exists.

We cannot change the default behavior of load as it would break BC.

FWIW, I don't agree with this. It's fine to change load semantics, as long as bitcode autoupgrade is handled correctly. I'd go even further and say that at least long term, any solution that does not have a plain load instruction return poison for uninitialized memory will be a maintenance mess.

I think the core problem that prevents us from moving in this direction is that we have no way to represent a frozen load at all (as freeze(load) will propagate poison before freezing). If I understand correctly, you're trying to solve this by making *all* loads frozen loads, and use load !noundef to allow dropping the freeze. I think this would be a very bad outcome, especially as we're going to lose that !noundef on most load speculations, and I don't think we want to be introducing freezes when speculating loads (e.g. during LICM).

I expect that the path of least resistance is going to be to support bitwise poison for load/store/phi/select and promote it to full-value poison on any other operation, allowing a freezing load to be expressed as freeze(load).

Please let me know if I completely misunderstood the scheme you have in mind...

In D131628#3897684, @gulfem wrote:

How should we proceed on this? AFAICT, we did not get a clear answer about the semantics of leaf attribute for LTO from LTO semantics for __attribute__((leaf)).
Would that be ok to drop nocalback attribute during linking? There is an LLVM bug now (Missing LangRef documentation for nocallback), and leaf attribute support will be reverted if we cannot come up with the description.

In D134410#3895077, @nikic wrote:

In D134410#3894995, @nlopes wrote:

We wanted this patch to make us switch uninitialized loads to poison at will, since they become UB. In practice, this helps us fixing bugs in SROA and etc without perf degradation.

Can you elaborate on this? I don't see how this is necessary for switching uninitialized loads to poison.

In D63401#3894996, @arsenm wrote:

In D63401#3894885, @nlopes wrote:

It seems this patch contradicts the proposed LangRef patch. The LangRef patch says that you can't assume that a volatile operation yields the same result in different spaces.
And I think that's a fair condition. Otherwise we need to document that constraint for hardware vendors and change LangRef. Pick one of the patches basically :)

This isn't contradictory. This is leaving the address space untouched, and doing the other simplifications SROA does (like replace byte arrays with int). A non-volatile access in this case would have been rewritten to point to the original alloca address space

In D134410#3893918, @nikic wrote:

I think adding this under a default-disabled flag is fine for evaluation purposes, but I have doubts that we will ever be able to enable this by default. There is a lot of code out there assuming that copying uninitialized data around is fine.

It seems this patch contradicts the proposed LangRef patch. The LangRef patch says that you can't assume that a volatile operation yields the same result in different spaces.
And I think that's a fair condition. Otherwise we need to document that constraint for hardware vendors and change LangRef. Pick one of the patches basically :)

I like this latest writing. But please wait for another reviewer to make sure I didn't miss smth.

In D136882#3891143, @nikic wrote:

I'm not completely convinced that this is correct. Note that icmp on pointers is a pure bitwise comparison, it is not affected by provenance. I think it's pretty clear that ptr null and ptr addrspace(3) @lds are distinct underlying objects, with distinct provenance, and that's all that matters to alias analysis. But it's less obvious to me that casting ptr null to ptr addrspace(3) may not produce a bit pattern that just so happens to match ptr addrspace(3) @lds.

@nlopes This silently fails to compile in the online version: https://alive2.llvm.org/ce/z/XC2RGM Locally it prints:

ERROR: Unsupported instruction:   %__constexpr_0 = icmp eq ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), @lds
ERROR: Unsupported instruction:   ret i1 icmp eq (ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(3) @lds)
ERROR: Could not translate 'src' to Alive IR

Would be great if these errors would show up online as well. Maybe the problem is that the error gets printed to stdout rather than stderr?

In D136284#3874755, @jamieschmeiser wrote:

In D136284#3874614, @nlopes wrote:

In D136284#3874596, @jamieschmeiser wrote:

In D136284#3874492, @nikic wrote:

At least in C++, working with uninitialized memory is pretty much always immediate undefined behavior, see https://eel.is/c++draft/basic.indet for the relevant wording. The only exception are "copy-like" operations on unsigned character types, which comparisons do not fall under.

I believe the C specification is less clear cut about this, but Clang and LLVM assume basically the same to also hold for C code.

What version of the C++ standard is this? Every version that I have seen has basics as section 3 and I cannot find this section, nor anything similar. Section 6 is Statements.

That discussion is orthogonal to this patch.
This patch is not desired because it's not needed per the current LLVM IR semantics. If you want to change something, you need to start by proposing a change to the LLVM IR semantics. You'll need to justify why it's needed, why it's correct, the perf impact, how to make it backwards compatible, why it's better than the proposals over the table right now.

Anyway, a patch like this solves no problem. LLVM allows loads to be duplicated. Your patch does nothing to prevent that and to ensure all loads see the same value. The issue is way more complicated than what this patch implies.

I'm not trying to flog a dead horse (I've already abandoned this) but I am trying to understand this statement. I do not dispute that there may be other situations similar to this but, assuming that we did want to ensure that the loads had the same value, why is freezing them at this point not the correct thing to do? Whether they are poison or undef, freezing them would ensure that they compare equal. Yes, I understand it may have performance impacts, there may be better ways, etc. But, ignoring all that, isn't this exactly what freeze is designed for?

In D136284#3874596, @jamieschmeiser wrote:

In D136284#3874492, @nikic wrote:

At least in C++, working with uninitialized memory is pretty much always immediate undefined behavior, see https://eel.is/c++draft/basic.indet for the relevant wording. The only exception are "copy-like" operations on unsigned character types, which comparisons do not fall under.

I believe the C specification is less clear cut about this, but Clang and LLVM assume basically the same to also hold for C code.

What version of the C++ standard is this? Every version that I have seen has basics as section 3 and I cannot find this section, nor anything similar. Section 6 is Statements.

Please avoid using UndefValue::get as much as possible as we are trying to get rid of undef. Please use PoisonValue whenever possible.
Thank you!

However, multiple loads of the same memory must be equal

In D136097#3863230, @spatel wrote:

In D136097#3863158, @nlopes wrote:

In D136097#3862946, @spatel wrote:

In D136097#3862928, @kpn wrote:

It looks like by 754 sec 7.3 a (-x)/(+0) == -Inf, but the nsz is documented as saying the sign of the result is "insignificant", so I think we're good here.

LGTM

"nsz" means the sign of a zero result is insignificant, not any result. -42.0 / 0.0 should be -Inf, not +Inf?

It's documented as ignoring the sign of a zero argument or a zero result.
Thus, when nsw is involved, we should consider the result of -42.0 / 0.0 and -42.0 / -0.0. This case the 0.0 is a constant, but that doesn't matter.

Alive2 is correct here AFAICT.

Ah, thanks for clearing that up. The "nsz" wording has always been a less solid than I'd like, but I'm not sure how to change it.
I suspect this transform would cause some fallout with users because it's a stretch to go from -0.0 isn't different than 0.0, therefore, -Inf isn't different than +Inf. There was a suggestion in a previous discussion that we really need entirely new FP types to use with fast-math-flags, so it's clear that -0.0, Inf, or NaN values simply do not exist when the corresponding compile flag is used, but I don't think anyone has looked seriously at implementing that. There's more discussion about the general problem here:
https://github.com/llvm/llvm-project/issues/51601

In D136097#3862946, @spatel wrote:

In D136097#3862928, @kpn wrote:

It looks like by 754 sec 7.3 a (-x)/(+0) == -Inf, but the nsz is documented as saying the sign of the result is "insignificant", so I think we're good here.

LGTM

"nsz" means the sign of a zero result is insignificant, not any result. -42.0 / 0.0 should be -Inf, not +Inf?

In D136055#3861580, @nikic wrote:

This is correct as specified, but I think there were plans to change !range and !nonnull to return poison, and require an additional !noundef to make them immediate UB, which is the way it works for attributes. I'm not sure what the state of that is though.

In D135451#3849630, @bmahjour wrote:

In D135451#3844084, @efriedma wrote:

As I understand it integer divide by zero is considered undefined behaviour in C while the same may not be true in other languages. Furthermore presence of side effects may be dependent on other configurations including target hardware (eg default treatment on Power vs x86).

The LLVM IR rule is more driven by the behavior of the instructions on various targets. If a target only has a trapping divide, we'd need to wrap it in control flow to implement a non-trapping divide. And particularly for signed divide, that check isn't cheap. We tend to prefer poison where it makes sense (for example, out-of-bounds shifts).

Frontends can always use control flow to get whatever user-visible behavior they want. (For example, the Rust divide operator panics if you divide by zero.)

To allow more flexibility we could either leave the IR neutral and let the optimizer decide based on config info (eg. TTI)

We try to avoid making core IR semantics depend on TTI. Not that we can completely ignore target differences when writing IR optimizations, but we want to keep IR understandable without reference to target-specific semantics.

I mean, it would be self-consistent to write in LangRef something like "whether division by zero is undefined behavior, or defined to produce a poison value, depends on the current target/current subtarget/bitwith of the operation/current moon cycle". But I don't want to go there. If the rules are the same across all targets, it's easier to understand, and easier to implement tools like Alive2 to validate transforms.

I'm sorry but I don't quite follow the logic that tries to justify the current design. On the one hand the IR rules are driven by target requirements, and at the same time you avoid making IR semantics dependent on TTI (which is the proper way to query about target requirements). They seem like recipes for the exact problem we are dealing with here, ie there are target-specific assumptions baked into the IR that are not configurable.

I would also prefer to not go this route.
Alternatives include using predicated builtins, stick to pre-headers, or introduce a new intrinsic that yields poison on division by zero and/or INT_MAX/-1.