Fix a small typo in PyFloat8E4M3B11FNUZType

LGTM

LGTM

Is there a test which uses flags.read with an alloca to ensure that we obey the redzone rules?

Your example is different from mine as it nests the constexpr variable inside the function rather than having it at translation-unit scope.

In D117569#3258307, @zahiraam wrote:

In D117569#3257121, @majnemer wrote:

I have a question regarding how this work with respect to the dllimport semantics known by the linker.
IIUC, we will now allow a program like:

extern int __declspec(dllimport) dll_import_int;
constexpr int& dll_import_constexpr_ref = dll_import_int;
int& get() {
    return dll_import_constexpr_ref;
}

Here, get will load dll_import_constexpr_ref. However, what will dll_import_constexpr_ref hold? It ought to hold the contents of __imp_?dll_import_int@@3HA. However, we can't dereference __imp_?dll_import_int@@3HA to get to its contents.

@majnemer Thanks for the review.

This test case doesn't link with MSVC. It will generate this error:
Microsoft (R) Incremental Linker Version 14.29.30133.0
Copyright (C) Microsoft Corporation. All rights reserved.

/out:test1.exe
test1.obj
test1.obj : error LNK2001: unresolved external symbol "int dll_import_int" (?dll_import_int@@3HA)
test1.exe : fatal error LNK1120: 1 unresolved externals

The symbols generated with MSVC are:
07 00000000 UNDEF notype External | ?dll_import_int@@3HA (int dll_import_int)
015 00000000 SECT6 notype Static | ?dll_import_constexpr_ref@@3AEAHEA (int & dll_import_constexpr_ref)

Without this patch this test case errors. With this patch clang will generate these symbols:

010 00000000 UNDEF notype External | imp_?dll_import_int@@3HA (declspec(dllimport) int dll_import_int)
012 00000000 SECT5 notype External | ?dll_import_constexpr_ref@@3AEAHEA (int & dll_import_constexpr_ref)
013 00000000 UNDEF notype External | ?dll_import_int@@3HA (int dll_import_int)

and will error at link time with this error:
test1-f1f63b.o : error LNK2019: unresolved external symbol "declspec(dllimport) int dll_import_int" (imp_?dll_import_int@@3HA) referenced in function "int & __cdecl get(void)" (?get@@YAAEAHXZ)
test1-f1f63b.o : error LNK2001: unresolved external symbol "int dll_import_int" (?dll_import_int@@3HA)
a.exe : fatal error LNK1120: 2 unresolved externals

I think that's the behavior expected, right?

I have a question regarding how this work with respect to the dllimport semantics known by the linker.
IIUC, we will now allow a program like:

extern int __declspec(dllimport) dll_import_int;
constexpr int& dll_import_constexpr_ref = dll_import_int;
int& get() {
    return dll_import_constexpr_ref;
}

LGTM

Do we also need to test for __aarch64__? https://developer.arm.com/documentation/dui0774/g/chr1383660321827 says that __arm__ is only defined for 32-bit targets.

Looks great! Please give others some time to review it as it is a holiday season...

I wonder if we should have different behavior for MSVC targets.

In D114425#3217478, @Quuxplusone wrote:

In D114425#3216802, @majnemer wrote:

OOC, how hard would it be to generalize this builtin a little? It is nice that we have builtins like __builtin_add_overflow which do the right thing regardless of their input.

It seems like it would be nice if we started to expose more intrinsics which did the right thing regardless of operand width; another bonus is that it composes well with language features like _BitInt.

IMHO such builtins are nice iff the programmer can be 100% sure that the compiler will interpret them the same way as a human reader. __builtin_add_overflow is easy because its first two arguments are "mathematical integers" (where integer promotion doesn't matter) and its third argument is a pointer (where integer promotion can't happen). So you can really throw any combination of types at it, and it'll do "the right thing" https://godbolt.org/z/sa7b894oa (although I admit I was surprised that this worked).
For a hypothetical __builtin_bswap, you would probably need a similar pointer-based interface like

short s16 = 0xFEDC;
__builtin_bswap(&s16);  // hypothetically
assert(s16 == 0xDCFE);

assert(__builtin_bswap16(s16) == 0x0000DCFE);
assert(__builtin_bswap32(s16) == 0xDCFEFFFF);  // the problem to solve: s16 eagerly promotes to int, which changes the result

The downside is that the pointer-based interface is less ergonomic than today's value-based signatures, and probably worse codegen at -O0 (because the programmer has to materialize the operand into a named variable, and then the compiler won't remove that variable because you might want to debug it). The upside (as you said) is that a generic builtin could work with _ExtInt types and so on.

In D115456#3217595, @momo5502 wrote:

In D115456#3216811, @majnemer wrote:

This is looking great! Just a few more questions.

What is the behavior with something like:

thread_local int x = 2;
int f() {
  return x;
}

I'm wondering if we need to move this logic into the generic C++ ABI implementation.

The MS compiler only emits the dynamic initializers for variables with constructors/destructors, just like it is currently done here for the Itanium ABI.
I also thought about adopting that behaviour, but I think threre are edge-cases when triggering dynamic TLS initialization even for constant variables is useful.
For example there might be custom TLS callbacks that can affect the value of this variable.

If desired, I can change it to match the behaviour of MS, but I thought it could be beneficial to diverge in this case.

This is looking great! Just a few more questions.

OOC, how hard would it be to generalize this builtin a little? It is nice that we have builtins like __builtin_add_overflow which do the right thing regardless of their input.

Is this a new feature in MSVC? Seems like it might be. If so, should it be predicated on isCompatibleWithMSVC(1925) or something?

LGTM

LGTM

It is difficult remembering exactly what my thought process was way back when but the redzone calculation seems very likely...

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

In D77777#1975618, @hliao wrote:

In D77777#1975406, @tra wrote:

In D77777#1975178, @hliao wrote:

the 1st argument in llvm.nvvm.texsurf.hande.internal or the 2nd one in llvm.nvvm.texsurf.handle must be kept as an immediate or constant value, i.e. that global variable. However, optimizations will find common code in the following

if (cond) {
  %hnd = texsurf.handle.internal(@tex1);
} else {
  %hnd = texsurf.handle.internal(@tex2)
}
= use(%hnd)

and hoist or sink it into

if (cond) {
  %ptr = @tex1;
} else {
  %ptr = @tex2;
}
%hnd = texsurf.handle.intenal(%ptr);
= use(%hnd)

The backend cannot handle non immediate operand in texsurf.handle. The similar thing happens to read.register as well as it also assumes its argument is always an immediate value.

I wonder if we can use token types to represent the handle? https://reviews.llvm.org/D11861
@majnemer -- would this use case be suitable for the token type?

If we still could make PHI over token, it canont serve this purpose. Check llvm::canReplaceOperandWithVariable for operand for details.

LGTM

Please upload with full context.

LGTM

LGTM

LGTM

In D64141#1568924, @ThomasR wrote:

In D64141#1568896, @jmolloy wrote:

Most of this looks mechanical; I don't think the MVT changes should be contentious. However, the "is65536BitVector()" predicates are starting to get a bit unwieldy to read.

Does an out of tree target need these predicates? I feel they're sufficiently unreadable to either change the format ("is64kBitVector"?) or elide them for larger types.

Cheers,

James

I agree, it is not really needed I added it for consistency. I don't mind changing it to the format suggested. Which ones do you think should be changed?

LGTM

LGTM

In D53765#1367315, @craig.topper wrote:

In D53765#1367170, @majnemer wrote:

Do you have any tests which perform a callbr with a normal function target rather than inline asm?

No, and Verifier.cpp currently errors for it.

Do you have any tests which perform a callbr with a normal function target rather than inline asm?

LGTM

LGTM