isSafeToSpeculativelyExecute seems not safe to use from SimplifyCFG

[vtjnash]

The llvm::isSafeToSpeculativelyExecute function seems to be only safe to use when not speculating the result of moving instructions, since it may consider attributes that will get discarded during moving it later (e.g. dereferenceable). However, it can be seen that SimplifyCFG:: dominatesMergePoint tries to use it exactly in that way. Otherwise, like seen in the small snippet below, that can allow violating the semantics of attributes of the IR which, won't be valid after speculating is done (which is expected to discard those attributes).

The invalid speculation appears to happen here:

llvm-project/llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Lines 461 to 465 in b592917

	// Okay, it looks like the instruction IS in the "condition". Check to
	// see if it's a cheap instruction to unconditionally compute, and if it
	// only uses stuff defined outside of the condition. If so, hoist it out.
	if (!isSafeToSpeculativelyExecute(I))
	return false;

This can be observed by running opt -passes=simplifycfg this.ll and observing that it returns IR with only basic block, which violates the semantics of %4 being a conditional load in the original IR specification (%3 is not dereferencable after hoisting for foldTwoEntryPHINode):

define i64 @julia_collect_81(i1 %0, ptr nonnull align 8 dereferenceable(8) %1) {
top:
  br i1 %0, label %L82, label %L85

L82:
  %2 = load ptr, ptr %1, align 8, !dereferenceable !1, !align !1
  %3 = load i64, ptr %2, align 8
  br label %L85

L85:
  %4 = phi i64 [ %3, %L82 ], [ 0, %top ]
  ret i64 %4
}

!0 = !{}
!1 = !{i64 8}

What seems the best course of action to fix this pass mis-speculation? I think LICM and LoopRotationUtils and SpeculativeExecution may share same issue but SpeculativeExecution has a private AllPrecedingUsesFromBlockHoisted check that it appears to have been using to avoid this issue (present since that pass was introduced in 154eb5a)

[nikic]

For reference, the result of the transform is:

define i64 @julia_collect_81(i1 %0, ptr nonnull align 8 dereferenceable(8) %1) {
  %2 = load ptr, ptr %1, align 8, !align !0
  %3 = load i64, ptr %2, align 8
  %4 = select i1 %0, i64 %3, i64 0
  ret i64 %4
}

!0 = !{i64 8}

Which is correct. The poison generating !align attribute was preserved, while the UB generating !dereferenceable attribute was dropped.

I was not able to parse what problem this issue is trying to report beyond that.

[nikic]

Okay, now I get what this is about. It's not about the first load with the metadata, it's about the fact that the second load also gets speculated, based on the metadata the first load has before it gets dropped.

[nikic]

I think the clean way to fix this would be to make use of CxtI. We should only make use of !dereferenceable information that dominates the location we're speculating to. If we're checking multiple instructions before speculating, then the necessary dominance relationship will not be there. If CxtI is nullptr we should assume an arbitrary speculation target and never use the information.

May be a bit trickier than that in practice because a) I don't think we pass CxtI in most cases right now and b) the !dereferenceable handling is inside getPointerDereferenceableBytes() so we probably need yet another Can flag?

[llvmbot]

@llvm/issue-subscribers-julialang

Author: Jameson Nash (vtjnash)

The `llvm::isSafeToSpeculativelyExecute` function seems to be only safe to use when not speculating the result of moving instructions, since it may consider attributes that will get discarded during moving it later (e.g. dereferenceable). However, it can be seen that `SimplifyCFG:: dominatesMergePoint` tries to use it exactly in that way. Otherwise, like seen in the small snippet below, that can allow violating the semantics of attributes of the IR which, won't be valid after speculating is done (which is expected to discard those attributes).

The invalid speculation appears to happen here:

llvm-project/llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Lines 461 to 465 in b592917

	// Okay, it looks like the instruction IS in the "condition". Check to
	// see if it's a cheap instruction to unconditionally compute, and if it
	// only uses stuff defined outside of the condition. If so, hoist it out.
	if (!isSafeToSpeculativelyExecute(I))
	return false;

This can be observed by running opt -passes=simplifycfg this.ll and observing that it returns IR with only basic block, which violates the semantics of %4 being a conditional load in the original IR specification (%3 is not dereferencable after hoisting for foldTwoEntryPHINode):

define i64 @<!-- -->julia_collect_81(i1 %0, ptr nonnull align 8 dereferenceable(8) %1) {
top:
  br i1 %0, label %L82, label %L85

L82:
  %2 = load ptr, ptr %1, align 8, !dereferenceable !1, !align !1
  %3 = load i64, ptr %2, align 8
  br label %L85

L85:
  %4 = phi i64 [ %3, %L82 ], [ 0, %top ]
  ret i64 %4
}

!0 = !{}
!1 = !{i64 8}

What seems the best course of action to fix this pass mis-speculation? I think LICM and LoopRotationUtils and SpeculativeExecution may share same issue but SpeculativeExecution has a private AllPrecedingUsesFromBlockHoisted check that it appears to have been using to avoid this issue (present since that pass was introduced in 154eb5a)