This patch fixes pr45956 (https://bugs.llvm.org/show_bug.cgi?id=45956 ).
To minimize its impact to the quality of generated code, I suggest enabling
this only for LTO as a start (it has two JumpThreading passes registered).
This patch contains a flag that makes JumpThreading enable it.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a few patches that are not uploaded (that are short, as other patches do), but making tests for those takes some time.
I'll upload them after working tests for those are prepared.
I'm not sure about your plan for enabling this. You're going to get very little practical test coverage enabling this for "LTO". (Big projects are much more likely to be using ThinLTO.) And long-term, we don't to have a "please miscompile my code" argument for the JumpThreading pass.
llvm/lib/Transforms/Scalar/JumpThreading.cpp | ||
---|---|---|
2811 | I suspect we don't want to change this. Even if "freeze" version of the transform is legal with msan, it probably reduces the quality of msan diagnostics. |
llvm/lib/Transforms/Scalar/JumpThreading.cpp | ||
---|---|---|
2811 | Actually, I think this would reintroduce https://bugs.llvm.org/show_bug.cgi?id=45220, because the current handling of freeze instruction in MSan is to check (and report) the argument and mark the result as initialized / defined. Is this wrong? Should we instead ignore the input shadow in freeze? In that case, I agree, this change will make MSan miss some issues. |
llvm/lib/Transforms/Scalar/JumpThreading.cpp | ||
---|---|---|
2811 | If msan diagnoses uninitialized arguments to freeze, that defeats the point of freeze: the behavior should be defined even if the operand is undef/poison. Probably we want to avoid performing transforms that require introducing freeze before msan instrumentation is inserted. But I think if msan does see a freeze, it's probably better to respect it, as opposed to producing a false-positive diagnostic. |
As concerned, the argument should be removed & select should be unfolded with freeze by default in the long term.
By using this argument and gradually enabling this, we can control the speed of its increasing interaction with other optimizations.
Enabling this in LTO first might have a small impact, but I think having a benign start is good. Also, another patch that inserted freeze, D76483 , was reported a performance change with LTO enabled.
The progress isn't fast yet, but I believe that once we have enough optimizations/analyses updated to deal with freeze, fixing remaining bugs with freeze should be much easier.
llvm/lib/Transforms/Scalar/JumpThreading.cpp | ||
---|---|---|
2811 | Yes, I agree that msan should ignore the argument of freeze's operand. |
With the last patch applied, the performance change is within a reasonable range, except 523.xalancbmk_r . However, on my machine, the result of 523.xalancbmk_r varies sometimes without clear reason.
Ping
Shall we do the full insertion of freeze after noundef-relevant patches (D81678 and a few patches adding noundef to library functions) are fully landed?
Until then, I think limiting this to LTO is necessary because this will introduce many instructions that will interact with many optimizations in O3.
LGTM
I'm still a little skeptical about the short-term plan with LTO, but I'm not strongly against it.
I suspect we don't want to change this. Even if "freeze" version of the transform is legal with msan, it probably reduces the quality of msan diagnostics.