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.
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 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.
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?
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.
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.
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.
Please separate and land this assert.
This may accidentally change semantics for struct typed loads, I'm not sure.
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.
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.
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.
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.