Page MenuHomePhabricator

Test that volatile load type isn't changed
ClosedPublic

Authored by jfb on Mar 4 2020, 1:52 PM.

Details

Summary

As discussed in D75505, it's not particularly useful to change the type of a volatile load to/from floating-point/integer because it's followed by a bitcast, and it might lead to surprising code generation. Check that this doesn't generally happen.

Diff Detail

Event Timeline

jfb created this revision.Mar 4 2020, 1:52 PM
lebedev.ri edited the summary of this revision. (Show Details)Mar 4 2020, 2:09 PM

Can you please add a test where we load i64 (needs datalayout) and then losslessly bitcast it into pointer?

jfb updated this revision to Diff 248356.Mar 4 2020, 4:40 PM

Add i64/double test, as well as ptr/int

Okay, thank you for the patch.
Langref changes seem to have gone AWOL.
This doesn't guarantee that we don't do such change elsewhere other than instcombine though.

llvm/test/Transforms/InstCombine/volatile_load_cast.ll
4

This needs a comment, something like

; See https://reviews.llvm.org/D75644 and https://reviews.llvm.org/D75505#inline-688817
; Don't ever change the type of `volatile`-loaded value, keep bitcast afterwards.
jfb updated this revision to Diff 248615.Mar 5 2020, 3:18 PM
jfb marked an inline comment as done.

Add comment

jfb added a comment.Mar 5 2020, 3:19 PM

Odd, the previous update only had the second commit. I've squashed all 3 commits now, so it's all here :)

lebedev.ri accepted this revision.Mar 5 2020, 10:14 PM

If you say so.
Thanks for the patch.

This revision is now accepted and ready to land.Mar 5 2020, 10:14 PM