This is an archive of the discontinued LLVM Phabricator instance.

[PredicateInfo] Enable -reverse-iterate tests only for +Asserts builds
ClosedPublic

Authored by mgrang on Jun 1 2017, 12:07 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang created this revision.Jun 1 2017, 12:07 PM
davide edited edge metadata.Jun 1 2017, 12:08 PM

Why does this break?

Because -reverse-iterate is only defined when LLVM_ENABLE_ABI_BREAKING_CHECKS is enabled.

efriedma accepted this revision.Jun 1 2017, 1:40 PM

Looks fine, but please fix the commit message to note that -reverse-iterate only exists in +Asserts builds.

This revision is now accepted and ready to land.Jun 1 2017, 1:40 PM
davide added a comment.Jun 1 2017, 1:51 PM

Yes, you can add REQUIRES: asserts (and make sure it actually works).

See test/Transforms/LoopRotate/oz-disable.ll for an example.

mgrang added a comment.Jun 1 2017, 1:54 PM

Thanks @efriedma, @davide, @dberlin for the reviews. I will go ahead and split these two tests and guard with REQUIRES: asserts.

mgrang updated this revision to Diff 101132.Jun 1 2017, 3:58 PM
mgrang retitled this revision from [PredicateInfo] Remove -reverse-iterate from tests as it breaks on Release builds to [PredicateInfo] Enable -reverse-iterate tests only for +Asserts builds.
mgrang edited the summary of this revision. (Show Details)

Split -reverse-iterate tests and guarded them with REQUIRES: asserts.
I validated the tests with Release as well as Release+Asserts builds.

efriedma accepted this revision.Jun 1 2017, 4:01 PM

LGTM

chapuni added a subscriber: chapuni.Jun 1 2017, 4:04 PM

Would there be any cases with ABI_BREAKING_CHECKS enabled but ENABLE_ASSERTIONS=OFF?

I could introduce a new feature "REQUIRES: abi-breaking-checks"

mgrang added a comment.Jun 1 2017, 4:15 PM

Would there be any cases with ABI_BREAKING_CHECKS enabled but ENABLE_ASSERTIONS=OFF?

I could introduce a new feature "REQUIRES: abi-breaking-checks"

@chapuni Currently, I see that ABI_BREAKING_CHECKS is turned OFF only if it is explicitly set to FORCE_OFF. Otherwise, it is enabled if ASSERTS are enabled (in cmake/modules/HandleLLVMOptions.cmake).
I am not sure if there is some buildbot which explicitly turns ABI_BREAKING off.
Introducing REQUIRES: abi-breaking-checks is a good idea though.

chapuni accepted this revision.Jun 1 2017, 4:20 PM

ATM, "REQUIRES: asserts" will satisfy our usual cases.

Ping me if we require the feature "abi-breaking-checks".

mgrang added a comment.Jun 1 2017, 4:23 PM

ATM, "REQUIRES: asserts" will satisfy our usual cases.

Ping me if we require the feature "abi-breaking-checks".

Sure, thanks a lot!

This revision was automatically updated to reflect the committed changes.