In D49723#4116435, @Ayal wrote:

This is admittedly a couple of years old by now, but wonder about that other intended patch - Clang still seems to consider pointers in struct arguments to be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d

The patch is merged (again). Please let me know if it causes any regressions for in-tree targets or tests.

In D135462#4112048, @lkail wrote:

Is there any progress with this patch? This patch can fix our downstream test failure which is caused by insistent alignment between IR and backend, thus affects alias analysis.

• Fixed code style issues.

@tra, can you please check if it is OK to merge this patch?

• Rolled back changes for param address space: treat it as constant memory until cvta.param is supported.
• Changed tests to use opaque pointers.

Just to summarize, the current implementation of padding in libomptarget has a bug, but it was hidden until we changed default alignment of allocas in D135462. This bug is now gating D135462 and Pavel and I were trying to investigate it. @jdoerfert, @grokos, your feedback is very welcome, since we're not really familiar with the code in OpenMP runtime.

I think the current patch is the way to go: we want to treat LDU and LDG as regular instructions and exclude them from memory analysis. Therefore memory references are not needed and we can drop them.

• Rebased

Committed in rG6ff87fed441680f0d673186e602c6017fef68640

LGTM

It appears that the issue is caused by libomptarget mapping code that adds padding if the begin address does not have alignment of 8 (search for "alignment" in omptarget.cpp). The problem is that the padding is not applied consistently, so we may have a mapping without the padding (size == 8), and then get a request for a mapping with the padding (size == 12). Notice that we don't have Using a padding of 4 bytes for begin address for the first mapping, and we have it for the same exact pointer for the second mapping.

I checked OpenMP tests, and the difference is pretty much what was expected. This is what we have as an input (the patch does not change the IR):

In D135462#3998037, @ronlieb wrote:

hello , this commit is breaking our buildbot : could you address please ? https://lab.llvm.org/buildbot/#/builders/193

@efriedma, can you please check this patch again?

• Rebased.

• param pointers may alias global pointers.
• added more test cases for param.

@nikic, @tra, please let me know if any other changes are needed.

• Rebased.
• Replaced pointsToConstantMemory with getModRefInfoMask due to the API change.
• Removed DataLayout variable that was unused.

The patch looks fine to me. TEST_SUITE_RUN_UNDER is a great alternative, but just setting these env variables should be enough for now.
@jdenny, can you please take a look at this patch?

• Fixed typos

• Applied code review comments

Without the patch, @callee has alignment of 16, while the call site uses alignment of 4 for param memory.
We also need D134548 to fix handling of bitcasts.

LGTM.

This took a while, I'm sorry for not following up earlier.

• Rebased
• Removed calls to AAResultBase and the extra check for GV->isConstant.

In D135462#3843425, @efriedma wrote:

Given we're de-emphasizing the type of allocas, this probably makes sense? We should be careful that this doesn't have unexpected effects, though; decreasing the alignment of an alloca could affect code generation. (For example, see D134282.)

• Removed an outdated comment
• Added another testcase to AArch64/preferred-alignment.ll

A similar issue was discussed and fixed in D79532, where promotion of alignment caused stack realignment.
This patch now removes promotion of alignment, so we always use the specified alignment.

• Added ptxas RUN lines.
• Removed test case with .local global variable because ptxas does not support them.

LGTM, thanks!

Thanks a lot for the patch! It would be great to get this issue finally fixed. I assume that this is the main patch, other patches in the stack seem like just preparation/adjustments needed for this one to work.

In D119669#3486842, @tra wrote:

In D119669#3481191, @tra wrote:

So, prefixing with $ does not solve the problem completely.

Indeed it's still possible to break compilation for both clang and nvcc: https://godbolt.org/z/9h649eMMn

Sorry, this got pushed back by other things, but I'd like to return to this conversation. I've uploaded a patch to add a simple AA that takes NVPTX address spaces into account and implements pointsToConstantMemory (D124787). This patch should be useful on its own, but I'm no longer sure that it is enough to safely (performace-wise) add mayLoad to LDU/LDG instructions.

In D121727#3479832, @thakis wrote:

It looks very weird to me to have LLVM's test suite support calling some proprietary tool. I can see how this is useful, but imho it doesn't belong into LLVM itself.

This is both for practical and other reasons.

From a practical point of view, if we rely on this tool for testing in LLVM, it means we can't run LLVM's tests on platforms that that tool doesn't run on.

Merged 99333026607f.

I missed two other tests that failed because of the same issue: debug-file-loc.ll and debug-file-loc-only.ll.
@thakis tried to fix them in a1bb5719eca6 (thanks a lot!), but I'm not sure if it helped - we need two backslash characters in the regex.