Page MenuHomePhabricator

[GVN] teach ConstantFolding correct handling of non-integral addrspace casts
Needs ReviewPublic

Authored by vtjnash on Mar 22 2019, 5:54 PM.

Details

Reviewers
reames
loladiro
Summary
Here we teach the ConstantFolding analysis pass that it is not legal to replace a load of a bitcast constant (having a non-integral addrspace) with a bitcast of the value of that constant (with a different non-integral addrspace).

But also teach it that certain bit patterns are always known and convertable (a fact it already uses elsewhere). This requires us to also fix a globalopt test, since, after this change, LLVM is able to realize that the test actually is a valid transform (NULL is always a known bit-pattern) and so it doesn't need to emit the failure remarks for it.

Also simplify some of the negative tests for transforms by avoiding a type change in their bitcast, and add positive versions of the same tests, to show that they otherwise should work.

Diff Detail

Event Timeline

vtjnash created this revision.Mar 22 2019, 5:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 5:54 PM
reames requested changes to this revision.Mar 26 2019, 6:09 PM
reames added inline comments.
test/Transforms/GVN/non-integral-pointers.ll
184

Please revert the type change on this test. Feel free to add a separate test with the i64 type.

Actually, your test diffs in general are confusing. Please land your tests, and then rebase on top of them so that changes are visible. Please do NOT change existing tests without cause.

This revision now requires changes to proceed.Mar 26 2019, 6:09 PM
vtjnash updated this revision to Diff 196652.Apr 25 2019, 9:20 AM

bump. please review, thanks!

loladiro accepted this revision.Jun 25 2019, 2:08 PM

This looks fine to me now. @reames if you have a chance, I'd appreciate confirmation that your concerns are addressed as well.

reames requested changes to this revision.Jun 28 2019, 4:45 PM
reames added inline comments.
lib/Analysis/ConstantFolding.cpp
101

This line is incorrect. a) It volates the specification of the function (return ConstantExpr::getBitCast(C, DestTy);). b) for the ASs not to match this would have to be an addrspace cast, not a bitcast. Which shouldn't reach here.

lib/Transforms/Utils/VNCoercion.cpp
324

there's probably something missing here. You're removing an early exit without introducing a new one. Given we still need to bail in at least some cases, that's highly suspicious?

This revision now requires changes to proceed.Jun 28 2019, 4:45 PM
vtjnash updated this revision to Diff 207207.Jun 29 2019, 12:27 PM
vtjnash marked 2 inline comments as done.

FoldBitCast was being instructed to create invalid IR in several test cases. We happened to fold away them away before the verifier noticed, so this adds an explicit check, and corrects the caller FoldReinterpretLoadFromConstPtr to form valid IR when handling vectors of pointers with this method.

vtjnash added inline comments.Jun 29 2019, 12:31 PM
lib/Analysis/ConstantFolding.cpp
101

Good call. That makes sense. Several cases in the test suite do accidentally reach here. I'll fix the caller to ensure it only passes in valid inputs, and make this an assert here instead.

lib/Transforms/Utils/VNCoercion.cpp
324

It's really hard to write a correct early-exit here, as we need duplicate a substantial chunk of ConstantFoldLoadFromConstPtr (as the TODO below hints at) to prove which cases it knows how to handle correctly. This instead now leans on ConstantFolding to only create and return valid IR. I think dropping this makes it easier to test also, since we don't need to show that both the early-return here and the fall-through cases would give the right answers.

Bump. Fixed the other bug in handling vectors that you noted, and added an assert to catch that misuse.

reames requested changes to this revision.Jul 17 2019, 11:53 AM
reames added inline comments.
lib/Analysis/ConstantFolding.cpp
95

Please separate and land this assert.

331

This may accidentally change semantics for struct typed loads, I'm not sure.

543

I'm not following the need for this change.

Strong suggestion: Make the smallest possible change you can, and post that for review. You've changed the patch here multiple times which is making it challenging to give you meaningful review across revisions. Take the time to pull out helper functions if relevant, add asserts, whatever. Just find a way to make the correctness of each step self documenting.

This revision now requires changes to proceed.Jul 17 2019, 11:53 AM
vtjnash updated this revision to Diff 211001.Jul 21 2019, 9:29 AM
vtjnash marked 2 inline comments as done.
vtjnash edited the summary of this revision. (Show Details)

[GVN] teach ConstantFolding correct handling of non-integral addrspace casts

vtjnash updated this revision to Diff 211002.Jul 21 2019, 9:31 AM

[GVN] teach ConstantFolding correct handling of non-integral addrspace casts

Harbormaster completed remote builds in B35438: Diff 211002.
vtjnash marked 2 inline comments as done.Jul 21 2019, 9:36 AM
vtjnash added inline comments.
lib/Analysis/ConstantFolding.cpp
331

I don't see how that could happen: the while loop below repeated slices up C (aka SrcTy, aka SrcSize) until SrcSize==DestSize. If SrcSize < DestSize, then transitivity ensures that was never true. That should make sense too from a desired behavior standpoint, since if we're casting a small constant to a larger one, we're appending unknown (not necessarily undef) bits.

Since we need to catch the "isNullValue" obvious cases below to avoid regressing capabilities (and failing the GVN tests) while introducing the DL.isNonIntegralPointerType correctness check, we need this early-exit first to check the same size precondition that would get implied by the while loop.

543

It's the bug you mentioned above (this line ... violates the specification of the function). I've split off this fix into https://reviews.llvm.org/D65057.

bump: this should be ready to review now.

ping. With the other commits split out and landed, this is now much smaller at moving the isNonIntegralPointerType check into the correct place. This is still good to go from my perspective.