thanks for this Philip!

addressed review comments. Added more testcases (positive and countercases).

handle clang-tidy comment.

handles complex addressing mode using base and index operands. All test cases pass.
I plan to add more counter examples once the basic idea is agreed upon.

addressed review comments.

While working on a follow-up change, I realized we don't need to separate out whether we're handling simple or complex addressing mode based on whether we fault in zero page or not. Both are orthogonal issues. The reason I coupled them is because we still need a way to get the "total constant address" *if it exists* and make a distinction between when we succeeded to get one versus when there was no such constant address apart from the offset.

thanks Denis for your comments. I'll let others have a chance in review as well. Regarding broader audience today, implicit null checks works only with X86 (or rather that's what we test with). Also, none of the global APIs are changed, precisely because they are used in other parts of the code. Hence a new API is introduced and used currently only for implicit null checks. It is broad enough that we can reuse it in other passes if we wish to do so.

addressed review comments around using simpler form rather than isTied. Whitespace testcase changes avoided. NFC w.r.t. prev diff.

Code LGTM. quick glance at tests.

LGTM w/ comments inline.

LGTM. thanks for working on this! Apart from dropping all of the extra handling code in RS4GC and verifier (which itself is a really good reason for this support btw), I'm assuming this for optimizations which know about the deopt_op bundle (or gc_transition bundle) after relocations are made explicit? i.e., a general robustness patch.

thank you for the review!

addressed review comment about test case.

addressed review comments. Added unittest case.

Landed as bb308b020522420413c7d3f2989a88f2fc423c56.

Thanks Johannes. @fpetrogalli Francesco, I believe I've addressed your concern as well?

In D70107#2034194, @fpetrogalli wrote:

I don't see anything preventing this from being a function attribute as well.

HI @anna ,

apologies in advance for the late reply.

I don't think we want this. Imagine situation in which proto.h contains a scalar declaration foo marked with #pragma ompe declare simd. Suppose you include this header in 2 compilation units, one (sourceA.c) that can be compiled with -fopenmp-simd, and one (sourceB.c) that cannot be compiled with openmp - for any reason, for example because of some requirements on floating points computations. Then, you want to do some optimization that involves merging the two modules. The calls to foo in the IR generated from sourceB.c suddenly become vectorizable, which is wrong. So, overall, I think we should not attach this attribute to a function declaration.

I appreciate that what I described might be a remote possibility, but if we ever end up having to deal with it, it will be quite hard to fix.

Let me know if you have any question.

Kind regards,

Francesco

Hi @fpetrogalli,
Yes, but it should be orthogonal to that requirement. I think hiding the mangling behind a static function, while the demangling is exposed seems unintuitive. Any front-ends that need to attach the attribute will need this function exposed.

In D70107#2023372, @anna wrote:

Francesco @fpetrogalli, I was reusing this attribute for custom scalar functions and I have a fundamental question - why is it that this *only* a callsite attribute? I don't see anything preventing this from being a function attribute as well. The meaning for the function attribute being if "variant-abi" is present on the function, it implies all callsites calling this particular function has that attribute. Of course, if you have multiple units being finally linked, it is the responsibility of the front end to choose it it wants to add the function attribute or a callsite attribute. However, I strongly feel we should have support for both, because there are use cases where the vector shape etc does not change depending on the callsite. Was there a reason for not supporting it as a function attribute as well?

Francesco @fpetrogalli, I was reusing this attribute for custom scalar functions and I have a fundamental question - why is it that this *only* a callsite attribute? I don't see anything preventing this from being a function attribute as well. The meaning for the function attribute being if "variant-abi" is present on the function, it implies all callsites calling this particular function has that attribute. Of course, if you have multiple units being finally linked, it is the responsibility of the front end to choose it it wants to add the function attribute or a callsite attribute. However, I strongly feel we should have support for both, because there are use cases where the vector shape etc does not change depending on the callsite. Was there a reason for not supporting it as a function attribute as well?

In D70107#2019000, @fpetrogalli wrote:

In D70107#2018811, @anna wrote:

For my purposes, our front-end converts the code into IR and our use case works with adding the attribute at that point itself. I was hoping to actually avoid having the various vector declarations in the IR - frankly it's just for IR cleanliness and some compile time (depending on number of vector variants for each such scalar function) since the vector declarations won't typically be used until we do a pass for some form of vectorization (loop/SLP etc).

I understand - this was our original intent too. Add the attribute and build the vector function signature at compile time in the middle end, our of the scalar signature in the IR and the mangled string of the vector function name. We started this work last summer, until our intern @aranisumedh found out that it was not possible to retrieve the vector signatures in some cases [1]. Essentially, it turned out that only the frontend has enough information about the language types that are needed to build the correct signature in IR. hence, we decided the vector declaration to be required in the IR.

[1] See this: http://lists.llvm.org/pipermail/llvm-dev/2019-June/133225.html, and the follow up discussion. Let me know if you have any question!

what I was thinking of adding in inject-TLI-mappings or a separate pass such as "inject-vector-declarations" is:

if (call->hasAttr("variant-abi")) {
  getVectorVariantFromAttr(call, VariantNames)
  for (VName: VariantNames) 
    addVariantDeclaration(CI, VF, VName);
}

So, to that end, I think it would be better to make the property that the "vector name mentioned in the attribute should be present in the IR as a vector function declaration" as optional?

Sorry, I don't think we can do this, for the aforementioned reason, and for the sake of consistency. It would be bad to end up having code to handle the custom behavior of the attribute.

Thanks for the clarification here.

Thank you for the detailed and quick response @fpetrogalli.

In D70107#2018558, @fpetrogalli wrote:

In D70107#2018145, @anna wrote:

@fpetrogalli I have a high level question - why does this pass require that the function should be from one of the vector libraries (Mass, SMVL etc)? I'd like to piggy-back on the vector-function-abi-variant attribute to vectorize a call to a scalar function, once the front-end adds this attribute to the callsite. So, if we have the following code in IR, with a scalar call to a function trivially.vectorizable.func with the vector-function-abi-variant attribute, we will generate the vector function declarations and then LoopVectorizer/SLPVectorizer would do its thing.

Hi @anna ,

this is exactly what the attribute is for. This pass works only with the library recognized by the TLI as a shortcut implementation to avoid doing the codegeneration of the attribute for the library functions in the frontend.

As things are, if you run opt with -O2 on your IR, the code should be vectorized with a call to the vector version, if not for the fact that the IR is missing the declaration of the actual vector function. In fact, the vector-function-abi-variant attribute requires that all the mappings listed in the attribute are resolved by some declaration/definition in the IR.

Ah, interesting. I hadn't noticed that property of the attribute. So, basically, if we were to pass the attribute through the front-end, we're required to keep the vector declarations in the IR as well.

@fpetrogalli I have a high level question - why does this pass require that the function should be from one of the vector libraries (Mass, SMVL etc)? I'd like to piggy-back on the vector-function-abi-variant attribute to vectorize a call to a scalar function, once the front-end adds this attribute to the callsite. So, if we have the following code in IR, with a scalar call to a function trivially.vectorizable.func with the vector-function-abi-variant attribute, we will generate the vector function declarations and then LoopVectorizer/SLPVectorizer would do its thing.

In D76792#1989623, @yamauchi wrote:

Hi, some of our internal tests seems to have started crashing due to this patch.

Here's a bug report with a reduced test: https://bugs.llvm.org/show_bug.cgi?id=45590

Please take a look.

ping.

thanks for the review @jdoerfert . Will incorporate these changes and rebase on landed D76140.

In D76140#1957416, @reames wrote:

LGTM again, with minor change.

will update it.

p.s. Sorry for missing the functional issue the first time. All of the test changes should have made the issue obvious, but despite reading the LangRef description of signext, I somehow managed to miss the separation between ABI and optimization attributes.

thanks for the review Philip and pointing out the problem. All of us had missed the functional issue the first time around.

fixed missing code left out during rebase.

fixed buildbot failure. see above comments and added testcase test8.

whitelist valid return attributes and only add those. Added testcase for signext.

In D44429#1953211, @anna wrote:

Hi,

It looks like there's some UB in this code. Pls see this failure: https://reviews.llvm.org/D76140#1953201
I'm not sure how to go about fixing the MsgPackReader code.
@scott.linder Could you PTAL?

see above comment.

In D76140#1953201, @anna wrote:

I got a failure in one of the binaryFormats:
lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/MsgPackReader.cpp

Attributes 'zeroext and signext' are incompatible!
  %rev.i.i.i.i.i.i.i.i = tail call signext zeroext i16 @llvm.bswap.i16(i16 %ret.0.copyload.i.i.i.i) #6
in function _ZN4llvm7msgpack6Reader7readIntIsEENS_8ExpectedIbEERNS0_6ObjectE
fatal error: error in backend: Broken function found, compilation aborted!

I believe this just showcases undefined behaviour since we were having a returnValue (i.e. call) with an incomptable attribute compared to the return attribute on the callsite.

It looks like there's some UB in this code. Pls see this failure: https://reviews.llvm.org/D76140#1953201
I'm not sure how to go about fixing the MsgPackReader code.
@scott.linder Could you PTAL?

I got a failure in one of the binaryFormats:
lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/MsgPackReader.cpp

Attributes 'zeroext and signext' are incompatible!
  %rev.i.i.i.i.i.i.i.i = tail call signext zeroext i16 @llvm.bswap.i16(i16 %ret.0.copyload.i.i.i.i) #6
in function _ZN4llvm7msgpack6Reader7readIntIsEENS_8ExpectedIbEERNS0_6ObjectE
fatal error: error in backend: Broken function found, compilation aborted!

addressed review comments. Added two test cases: deref value being different, inlined callee body better optimized compared to callee.

addressed review comments

NFC w.r.t prev diff. Use VMap.lookup instead of a lambda which does the same.

Just chiming in here. We are planning to use addrspacecast for pointer width conversion (32 to 64 bit and vice-versa). Does not change the memory in anyway, i.e. if we lowered the addrspacecast into an IR call, that call could be marked argmemonly readnone. However, it is *not* a simple sign-extend/truncate either.
I've added some comments inline. Also, I think we have the scope to improve optimizations for addrspacecast at IR level, which is currently limited because different targets have varying lowering for addrspacecast.

ping.

Noticed while adding couple more tests, there were 2 bugs:

use isGuaranteedToTransferExecutionToSuccessor instead of MayThrow

refactored code, made asserts cleaner.

fixed clang tests. rot-intrinsics.c testcase has 5 different RUNs with 3 prefixes. Depending on target-triple, the attribute is added to the caller, so I've disabled the optimization for that specific test with -update-return-attrs=false

Will avoid confusing usage with cleaner asserts.

addressed review comments. fixed CI failure related to clang-tidy.

In D76140#1930078, @reames wrote:

In D76140#1927909, @anna wrote:

the test failures are related to attributes now being added to the calls in clang tests. All of these tests are using llvm intrinsics. Should we just disable this for intrinsic calls for now? Not sure how to update clang tests in the same patch as llvm project. Should be doable though.

Er, git is monorepo. Those tests should be checked out alongside your LLVM copy. You just need to build clang. You can and should simply update the tests in the same patch.

ah yes, I'd build llvm within it's own subdir without enabling any other projects. Build worked. thanks.