This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Fix for TBAA information of pointer after addresspacecast
ClosedPublic

Authored by romanovvlad on Dec 4 2018, 3:19 AM.

Diff Detail

Event Timeline

romanovvlad created this revision.Dec 4 2018, 3:19 AM
rjmccall accepted this revision.Dec 4 2018, 8:19 PM

LGTM. Although this also fixes bugs with unaligned pointers that you might consider adding tests for.

This revision is now accepted and ready to land.Dec 4 2018, 8:19 PM

What bugs with unaligned pointers do you mean?

The code was dropping all the original l-value information, which includes alignment. So if you start with an l-value that refers to under-aligned memory (or over-aligned), we were losing that information on the resulting l-value. The test case would be something like:

struct __attribute__((packed)) A { int x; };
int test(__global A *a) {
  return static_cast<__generic A &>(*a).x; // this load should have alignment 1
}

Added new test case + formatting.

Oh, sorry, that wasn't a good example because that's still the natural alignment for A. It should be static_cast<__generic int &>(GPtr->X).

I'm getting the same IR for both examples you provided. And for both load of X value is aligned on 4 bytes wo patch and on 1 byte with.

I'm getting the same IR for both examples you provided. And for both load of X value is aligned on 4 bytes wo patch and on 1 byte with.

That's odd, because the natural type alignment for A ought to be 1, since it's a packed type. What does the AST look like?

romanovvlad added a comment.EditedDec 6 2018, 10:08 AM

Sorry, I was compiling the first version two times.
With second example clang crashes :)

Yes it's 1, but after addrspacecast alignment becomes not struct A but alignment of the pointer

Oh! Yes, that's even more broken than I thought.

lib/CodeGen/CGExpr.cpp
4271–4272

This call to getAddressSpace() doesn't look right: we just constructed DestTy as an unqualified pointer type. It needs to be E->getType().getAddressSpace().

Backtrace:

Sorry. Can you post the result of passing -ast-dump instead of -emit-llvm to the compiler?

Sorry. Can you post the result of passing -ast-dump instead of -emit-llvm to the compiler?

Actually, nevermind, I don't need that. If you can fix the address-space issue I pointed out, I think this is ready to go.

rjmccall accepted this revision.Dec 6 2018, 12:15 PM

Thanks, LGTM.

romanovvlad marked an inline comment as done.Dec 7 2018, 1:11 AM

Could you merge it? I have no rights to do it.

@Anastasia Could you, please, merge the patch if it's OK with you?

This revision was automatically updated to reflect the committed changes.