[PATCH] D37460: [GVN] Prevent LoadPRE from hoisting through guards

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In https://reviews.llvm.org/D37460#866870, @efriedma wrote:

> We consider volatile loads and stores as potentially trapping as a courtesy to our users.  Strictly speaking, it's undefined behavior in C, but people sometimes intentionally trigger SIGSEGV using volatile loads, so we don't want to cause an unrelated crash before that happens.  Breaking that testcase should be fine.


I disagree.  This would be breaking optimizations *across* a volatile load, not breaking optimizations *of* a volatile load.  I think the isGuaranteedTo... scheme is probably the right one, but I think we got our semantics for that function wrong for volatile loads.

Given we've not documented volatile loads as potentially trapping, I don't think we should *partially* implement that semantic.  I'd actually like to have a faulting load concept in the IR, but hacking that in for volatile seems misguided.

For the moment, I'll suggest a compromise to unblock this review from the broader discussion.  Can we use the isGuaranteedTo... scheme, but additionally whitelist skipping over volatile loads?  That let's us make progress on this correctness issue while taking the volatile semantics discussion elsewhere.



================
Comment at: lib/Transforms/Scalar/GVN.cpp:2049
       ChangedFunction |= replaceOperandsWithConsts(&*BI);
+    if (IsGuard(*BI))
+      BlocksWithGuards.insert(BB);
----------------
I'm not sure building this set in RPO is sufficient in the face of cycles.  I believe the current PRE code would bail out of said cycles, but at minimum, that assumptions needs to be carefully documented and asserted for.

i.e. how do we know that BlocksWithGuards has been populated for each block we traverse backwards through?

In particular, if we extend this to isGuaranteedTo... and the scalar PRE case from https://reviews.llvm.org/D21041, it's not obvious to me this code would be correct.


https://reviews.llvm.org/D37460