[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Given the complexity here, I agree this is probably the best we can reasonably 
do.  Code and test changes LGTM.

That said, this is missing a release note.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86310/new/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> For assembly, I'm not really comfortable with the fix you're proposing; it 
> relies on the order in which functions are defined in assembly. I think LLVM 
> usually ends up emitting module-level inline assembly before generated code, 
> but it seems fragile to depend on that ordering. And the way the check is 
> written doesn't account for other differences in the way we normally generate 
> code for declarations vs. definitions, which I'm afraid might make the linker 
> unhappy.

Also, checking isUndefined() completely explodes if you use -fno-integrated-as, 
although I guess that's not common for Windows targets.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 557651.
efriedma added a comment.

Fix the calling convention for functions returning C++ classes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

Files:
  clang/lib/CodeGen/CGCXX.cpp
  llvm/include/llvm/IR/CallingConv.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.h
  llvm/lib/Target/AArch64/AArch64CallingConvention.td
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  llvm/lib/Target/AArch64/AArch64MCInstLower.h
  llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/lib/Target/AArch64/CMakeLists.txt
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
  llvm/test/CodeGen/AArch64/arm64ec-dllimport.ll
  llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
  llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
  llvm/test/CodeGen/AArch64/stack-protector-target.ll
  llvm/test/CodeGen/AArch64/win-alloca.ll

Index: llvm/test/CodeGen/AArch64/win-alloca.ll
===
--- llvm/test/CodeGen/AArch64/win-alloca.ll
+++ llvm/test/CodeGen/AArch64/win-alloca.ll
@@ -21,4 +21,4 @@
 ; CHECK-OPT: sub [[REG3:x[0-9]+]], sp, x15, lsl #4
 ; CHECK-OPT: mov sp, [[REG3]]
 ; CHECK: bl func2
-; CHECK-ARM64EC: bl __chkstk_arm64ec
+; CHECK-ARM64EC: bl "#__chkstk_arm64ec"
Index: llvm/test/CodeGen/AArch64/stack-protector-target.ll
===
--- llvm/test/CodeGen/AArch64/stack-protector-target.ll
+++ llvm/test/CodeGen/AArch64/stack-protector-target.ll
@@ -39,6 +39,6 @@
 ; WINDOWS-ARM64EC: adrp x8, __security_cookie
 ; WINDOWS-ARM64EC: ldr x8, [x8, :lo12:__security_cookie]
 ; WINDOWS-ARM64EC: str x8, [sp, #8]
-; WINDOWS-ARM64EC: bl  _Z7CapturePi
+; WINDOWS-ARM64EC: bl "#_Z7CapturePi"
 ; WINDOWS-ARM64EC: ldr x0, [sp, #8]
-; WINDOWS-ARM64EC: bl  __security_check_cookie_arm64ec
+; WINDOWS-ARM64EC: bl "#__security_check_cookie_arm64ec"
Index: llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
 
 define void @varargs_callee(double %x, ...) nounwind {
 ; CHECK-LABEL: varargs_callee:
@@ -44,7 +44,11 @@
 ; CHECK-NEXT:stp xzr, x30, [sp, #24] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x9, x8, [sp]
 ; CHECK-NEXT:str xzr, [sp, #16]
-; CHECK-NEXT:bl varargs_callee
+; CHECK-NEXT:.weak_anti_dep varargs_callee
+; CHECK-NEXT:  .set varargs_callee, "#varargs_callee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_callee"
+; CHECK-NEXT:  .set "#varargs_callee", varargs_callee@WEAKREF
+; CHECK-NEXT:bl "#varargs_callee"
 ; CHECK-NEXT:ldr x30, [sp, #32] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #48
 ; CHECK-NEXT:ret
@@ -81,7 +85,11 @@
 ; CHECK-NEXT:str x30, [sp, #48] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x9, x8, [sp]
 ; CHECK-NEXT:stp q0, q0, [sp, #16]
-; CHECK-NEXT:bl varargs_many_argscallee
+; CHECK-NEXT:.weak_anti_dep varargs_many_argscallee
+; CHECK-NEXT:  .set varargs_many_argscallee, "#varargs_many_argscallee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_many_argscallee"
+; CHECK-NEXT:  .set "#varargs_many_argscallee", varargs_many_argscallee@WEAKREF
+; CHECK-NEXT:bl "#varargs_many_argscallee"
 ; CHECK-NEXT:ldr x30, [sp, #48] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #64
 ; CHECK-NEXT:ret
Index: llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc 

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

For assembly, I'm not really comfortable with the fix you're proposing; it 
relies on the order in which functions are defined in assembly.  I think LLVM 
usually ends up emitting module-level inline assembly before generated code, 
but it seems fragile to depend on that ordering.  And the way the check is 
written doesn't account for other differences in the way we normally generate 
code for declarations vs. definitions, which I'm afraid might make the linker 
unhappy.

How important is that particular pattern?  I think most patterns involving 
assembly should be covered by some combination of naked functions, and 
functions defined in separate assembly files, both of which avoid weird 
questions about what the compiler should do when assembly tries to define a 
function symbol.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 557592.
efriedma added a comment.

Updated with feedback from @dpaoliello and additional internal testing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

Files:
  clang/lib/CodeGen/CGCXX.cpp
  llvm/include/llvm/IR/CallingConv.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.h
  llvm/lib/Target/AArch64/AArch64CallingConvention.td
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  llvm/lib/Target/AArch64/AArch64MCInstLower.h
  llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/lib/Target/AArch64/CMakeLists.txt
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
  llvm/test/CodeGen/AArch64/arm64ec-dllimport.ll
  llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
  llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
  llvm/test/CodeGen/AArch64/stack-protector-target.ll
  llvm/test/CodeGen/AArch64/win-alloca.ll

Index: llvm/test/CodeGen/AArch64/win-alloca.ll
===
--- llvm/test/CodeGen/AArch64/win-alloca.ll
+++ llvm/test/CodeGen/AArch64/win-alloca.ll
@@ -21,4 +21,4 @@
 ; CHECK-OPT: sub [[REG3:x[0-9]+]], sp, x15, lsl #4
 ; CHECK-OPT: mov sp, [[REG3]]
 ; CHECK: bl func2
-; CHECK-ARM64EC: bl __chkstk_arm64ec
+; CHECK-ARM64EC: bl "#__chkstk_arm64ec"
Index: llvm/test/CodeGen/AArch64/stack-protector-target.ll
===
--- llvm/test/CodeGen/AArch64/stack-protector-target.ll
+++ llvm/test/CodeGen/AArch64/stack-protector-target.ll
@@ -39,6 +39,6 @@
 ; WINDOWS-ARM64EC: adrp x8, __security_cookie
 ; WINDOWS-ARM64EC: ldr x8, [x8, :lo12:__security_cookie]
 ; WINDOWS-ARM64EC: str x8, [sp, #8]
-; WINDOWS-ARM64EC: bl  _Z7CapturePi
+; WINDOWS-ARM64EC: bl "#_Z7CapturePi"
 ; WINDOWS-ARM64EC: ldr x0, [sp, #8]
-; WINDOWS-ARM64EC: bl  __security_check_cookie_arm64ec
+; WINDOWS-ARM64EC: bl "#__security_check_cookie_arm64ec"
Index: llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
 
 define void @varargs_callee(double %x, ...) nounwind {
 ; CHECK-LABEL: varargs_callee:
@@ -44,7 +44,11 @@
 ; CHECK-NEXT:stp xzr, x30, [sp, #24] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x9, x8, [sp]
 ; CHECK-NEXT:str xzr, [sp, #16]
-; CHECK-NEXT:bl varargs_callee
+; CHECK-NEXT:.weak_anti_dep varargs_callee
+; CHECK-NEXT:  .set varargs_callee, "#varargs_callee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_callee"
+; CHECK-NEXT:  .set "#varargs_callee", varargs_callee@WEAKREF
+; CHECK-NEXT:bl "#varargs_callee"
 ; CHECK-NEXT:ldr x30, [sp, #32] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #48
 ; CHECK-NEXT:ret
@@ -81,7 +85,11 @@
 ; CHECK-NEXT:str x30, [sp, #48] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x9, x8, [sp]
 ; CHECK-NEXT:stp q0, q0, [sp, #16]
-; CHECK-NEXT:bl varargs_many_argscallee
+; CHECK-NEXT:.weak_anti_dep varargs_many_argscallee
+; CHECK-NEXT:  .set varargs_many_argscallee, "#varargs_many_argscallee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_many_argscallee"
+; CHECK-NEXT:  .set "#varargs_many_argscallee", varargs_many_argscallee@WEAKREF
+; CHECK-NEXT:bl "#varargs_many_argscallee"
 ; CHECK-NEXT:ldr x30, [sp, #48] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #64
 ; CHECK-NEXT:ret
Index: llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; 

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 557412.
efriedma added a comment.

Rebased


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

Files:
  clang/lib/CodeGen/CGCXX.cpp
  llvm/include/llvm/IR/CallingConv.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.h
  llvm/lib/Target/AArch64/AArch64CallingConvention.td
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/lib/Target/AArch64/CMakeLists.txt
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
  llvm/test/CodeGen/AArch64/arm64ec-dllimport.ll
  llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
  llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
  llvm/test/CodeGen/AArch64/stack-protector-target.ll
  llvm/test/CodeGen/AArch64/win-alloca.ll

Index: llvm/test/CodeGen/AArch64/win-alloca.ll
===
--- llvm/test/CodeGen/AArch64/win-alloca.ll
+++ llvm/test/CodeGen/AArch64/win-alloca.ll
@@ -21,4 +21,4 @@
 ; CHECK-OPT: sub [[REG3:x[0-9]+]], sp, x15, lsl #4
 ; CHECK-OPT: mov sp, [[REG3]]
 ; CHECK: bl func2
-; CHECK-ARM64EC: bl __chkstk_arm64ec
+; CHECK-ARM64EC: bl "#__chkstk_arm64ec"
Index: llvm/test/CodeGen/AArch64/stack-protector-target.ll
===
--- llvm/test/CodeGen/AArch64/stack-protector-target.ll
+++ llvm/test/CodeGen/AArch64/stack-protector-target.ll
@@ -39,6 +39,6 @@
 ; WINDOWS-ARM64EC: adrp x8, __security_cookie
 ; WINDOWS-ARM64EC: ldr x8, [x8, :lo12:__security_cookie]
 ; WINDOWS-ARM64EC: str x8, [sp, #8]
-; WINDOWS-ARM64EC: bl  _Z7CapturePi
+; WINDOWS-ARM64EC: bl "#_Z7CapturePi"
 ; WINDOWS-ARM64EC: ldr x0, [sp, #8]
-; WINDOWS-ARM64EC: bl  __security_check_cookie_arm64ec
+; WINDOWS-ARM64EC: bl "#__security_check_cookie_arm64ec"
Index: llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
 
 define void @varargs_callee(double %x, ...) nounwind {
 ; CHECK-LABEL: varargs_callee:
@@ -44,7 +44,11 @@
 ; CHECK-NEXT:stp xzr, x30, [sp, #24] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x9, x8, [sp]
 ; CHECK-NEXT:str xzr, [sp, #16]
-; CHECK-NEXT:bl varargs_callee
+; CHECK-NEXT:.weak_anti_dep varargs_callee
+; CHECK-NEXT:  .set varargs_callee, "#varargs_callee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_callee"
+; CHECK-NEXT:  .set "#varargs_callee", varargs_callee@WEAKREF
+; CHECK-NEXT:bl "#varargs_callee"
 ; CHECK-NEXT:ldr x30, [sp, #32] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #48
 ; CHECK-NEXT:ret
@@ -81,7 +85,11 @@
 ; CHECK-NEXT:str x30, [sp, #48] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x9, x8, [sp]
 ; CHECK-NEXT:stp q0, q0, [sp, #16]
-; CHECK-NEXT:bl varargs_many_argscallee
+; CHECK-NEXT:.weak_anti_dep varargs_many_argscallee
+; CHECK-NEXT:  .set varargs_many_argscallee, "#varargs_many_argscallee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_many_argscallee"
+; CHECK-NEXT:  .set "#varargs_many_argscallee", varargs_many_argscallee@WEAKREF
+; CHECK-NEXT:bl "#varargs_many_argscallee"
 ; CHECK-NEXT:ldr x30, [sp, #48] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #64
 ; CHECK-NEXT:ret
Index: llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc 

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 557335.
efriedma added a comment.

Revised so guest exit thunks actually work.  Thanks for the tip about the 
symbols; I forgot to look at the object file in addition to the generated 
assembly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

Files:
  clang/lib/CodeGen/CGCXX.cpp
  llvm/include/llvm/IR/CallingConv.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.h
  llvm/lib/Target/AArch64/AArch64CallingConvention.td
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/lib/Target/AArch64/CMakeLists.txt
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
  llvm/test/CodeGen/AArch64/arm64ec-dllimport.ll
  llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
  llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
  llvm/test/CodeGen/AArch64/stack-protector-target.ll
  llvm/test/CodeGen/AArch64/win-alloca.ll

Index: llvm/test/CodeGen/AArch64/win-alloca.ll
===
--- llvm/test/CodeGen/AArch64/win-alloca.ll
+++ llvm/test/CodeGen/AArch64/win-alloca.ll
@@ -21,4 +21,4 @@
 ; CHECK-OPT: sub [[REG3:x[0-9]+]], sp, x15, lsl #4
 ; CHECK-OPT: mov sp, [[REG3]]
 ; CHECK: bl func2
-; CHECK-ARM64EC: bl __chkstk_arm64ec
+; CHECK-ARM64EC: bl "#__chkstk_arm64ec"
Index: llvm/test/CodeGen/AArch64/stack-protector-target.ll
===
--- llvm/test/CodeGen/AArch64/stack-protector-target.ll
+++ llvm/test/CodeGen/AArch64/stack-protector-target.ll
@@ -39,6 +39,6 @@
 ; WINDOWS-ARM64EC: adrp x8, __security_cookie
 ; WINDOWS-ARM64EC: ldr x8, [x8, :lo12:__security_cookie]
 ; WINDOWS-ARM64EC: str x8, [sp, #8]
-; WINDOWS-ARM64EC: bl  _Z7CapturePi
+; WINDOWS-ARM64EC: bl "#_Z7CapturePi"
 ; WINDOWS-ARM64EC: ldr x0, [sp, #8]
-; WINDOWS-ARM64EC: bl  __security_check_cookie_arm64ec
+; WINDOWS-ARM64EC: bl "#__security_check_cookie_arm64ec"
Index: llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
 
 define void @varargs_callee(double %x, ...) nounwind {
 ; CHECK-LABEL: varargs_callee:
@@ -35,16 +35,20 @@
 ; CHECK-NEXT:sub sp, sp, #48
 ; CHECK-NEXT:mov x4, sp
 ; CHECK-NEXT:add x8, sp, #16
-; CHECK-NEXT:mov x9, #4617315517961601024
-; CHECK-NEXT:mov x0, #4607182418800017408
-; CHECK-NEXT:mov w1, #2
-; CHECK-NEXT:mov x2, #4613937818241073152
-; CHECK-NEXT:mov w3, #4
-; CHECK-NEXT:mov w5, #16
+; CHECK-NEXT:mov x9, #4617315517961601024 // =0x4014
+; CHECK-NEXT:mov x0, #4607182418800017408 // =0x3ff0
+; CHECK-NEXT:mov w1, #2 // =0x2
+; CHECK-NEXT:mov x2, #4613937818241073152 // =0x4008
+; CHECK-NEXT:mov w3, #4 // =0x4
+; CHECK-NEXT:mov w5, #16 // =0x10
 ; CHECK-NEXT:stp xzr, x30, [sp, #24] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x8, xzr, [sp, #8]
 ; CHECK-NEXT:str x9, [sp]
-; CHECK-NEXT:bl varargs_callee
+; CHECK-NEXT:.weak_anti_dep varargs_callee
+; CHECK-NEXT:  .set varargs_callee, "#varargs_callee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_callee"
+; CHECK-NEXT:  .set "#varargs_callee", varargs_callee@WEAKREF
+; CHECK-NEXT:bl "#varargs_callee"
 ; CHECK-NEXT:ldr x30, [sp, #32] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #48
 ; CHECK-NEXT:ret
@@ -71,17 +75,21 @@
 ; CHECK-NEXT:sub sp, sp, #64
 ; CHECK-NEXT:movi v0.2d, #
 ; CHECK-NEXT:mov x4, sp
-; CHECK-NEXT:mov x8, #4618441417868443648
+; CHECK-NEXT:mov x8, #4618441417868443648 // =0x4018
 ; CHECK-NEXT:add x9, sp, #16
 ; 

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 557315.
efriedma added a comment.

Updated with my attempt at guest exit thunks.

Seems to be working for simple cases, but I'm not sure this is actually working 
properly (I'm still seeing LNK1000 crashes).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

Files:
  clang/lib/CodeGen/CGCXX.cpp
  llvm/include/llvm/IR/CallingConv.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.h
  llvm/lib/Target/AArch64/AArch64CallingConvention.td
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/lib/Target/AArch64/CMakeLists.txt
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
  llvm/test/CodeGen/AArch64/arm64ec-dllimport.ll
  llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
  llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
  llvm/test/CodeGen/AArch64/stack-protector-target.ll
  llvm/test/CodeGen/AArch64/win-alloca.ll

Index: llvm/test/CodeGen/AArch64/win-alloca.ll
===
--- llvm/test/CodeGen/AArch64/win-alloca.ll
+++ llvm/test/CodeGen/AArch64/win-alloca.ll
@@ -21,4 +21,4 @@
 ; CHECK-OPT: sub [[REG3:x[0-9]+]], sp, x15, lsl #4
 ; CHECK-OPT: mov sp, [[REG3]]
 ; CHECK: bl func2
-; CHECK-ARM64EC: bl __chkstk_arm64ec
+; CHECK-ARM64EC: bl "#__chkstk_arm64ec"
Index: llvm/test/CodeGen/AArch64/stack-protector-target.ll
===
--- llvm/test/CodeGen/AArch64/stack-protector-target.ll
+++ llvm/test/CodeGen/AArch64/stack-protector-target.ll
@@ -39,6 +39,6 @@
 ; WINDOWS-ARM64EC: adrp x8, __security_cookie
 ; WINDOWS-ARM64EC: ldr x8, [x8, :lo12:__security_cookie]
 ; WINDOWS-ARM64EC: str x8, [sp, #8]
-; WINDOWS-ARM64EC: bl  _Z7CapturePi
+; WINDOWS-ARM64EC: bl "#_Z7CapturePi"
 ; WINDOWS-ARM64EC: ldr x0, [sp, #8]
-; WINDOWS-ARM64EC: bl  __security_check_cookie_arm64ec
+; WINDOWS-ARM64EC: bl "#__security_check_cookie_arm64ec"
Index: llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
 
 define void @varargs_callee(double %x, ...) nounwind {
 ; CHECK-LABEL: varargs_callee:
@@ -35,16 +35,20 @@
 ; CHECK-NEXT:sub sp, sp, #48
 ; CHECK-NEXT:mov x4, sp
 ; CHECK-NEXT:add x8, sp, #16
-; CHECK-NEXT:mov x9, #4617315517961601024
-; CHECK-NEXT:mov x0, #4607182418800017408
-; CHECK-NEXT:mov w1, #2
-; CHECK-NEXT:mov x2, #4613937818241073152
-; CHECK-NEXT:mov w3, #4
-; CHECK-NEXT:mov w5, #16
+; CHECK-NEXT:mov x9, #4617315517961601024 // =0x4014
+; CHECK-NEXT:mov x0, #4607182418800017408 // =0x3ff0
+; CHECK-NEXT:mov w1, #2 // =0x2
+; CHECK-NEXT:mov x2, #4613937818241073152 // =0x4008
+; CHECK-NEXT:mov w3, #4 // =0x4
+; CHECK-NEXT:mov w5, #16 // =0x10
 ; CHECK-NEXT:stp xzr, x30, [sp, #24] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x8, xzr, [sp, #8]
 ; CHECK-NEXT:str x9, [sp]
-; CHECK-NEXT:bl varargs_callee
+; CHECK-NEXT:.weak_anti_dep varargs_callee
+; CHECK-NEXT:  .set varargs_callee, "#varargs_callee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_callee"
+; CHECK-NEXT:  .set "#varargs_callee", varargs_callee@WEAKREF
+; CHECK-NEXT:bl "#varargs_callee"
 ; CHECK-NEXT:ldr x30, [sp, #32] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #48
 ; CHECK-NEXT:ret
@@ -71,17 +75,21 @@
 ; CHECK-NEXT:sub sp, sp, #64
 ; CHECK-NEXT:movi v0.2d, #
 ; CHECK-NEXT:mov x4, sp
-; CHECK-NEXT:mov x8, #4618441417868443648
+; CHECK-NEXT:mov x8, #4618441417868443648 // =0x4018
 ; CHECK-NEXT:add x9, 

[PATCH] D127221: [Clang] Enable -print-pipeline-passes in clang.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I don't really like `extern cl::opt PrintPipelinePasses;` (as opposed to 
implementing a proper driver option that calls a proper API); secret handshakes 
like this make it harder for people to write non-clang frontends.  But I won't 
block the patch just for that, I guess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127221/new/

https://reviews.llvm.org/D127221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156172/new/

https://reviews.llvm.org/D156172

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Maybe split the changes to reformat the unittests into a separate patch?

Otherwise, I'm happy with the current state of this patch, but probably someone 
more active in flang should approve.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158857: [clang][aarch64] Add support for the MS qualifiers __ptr32, __ptr64, __sptr, __uptr for aarch64

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

If you don't have autoupgrade, that means you can't link IR built with older 
versions of LLVM to IR built with newer versions.  
llvm::UpgradeDataLayoutString is designed to fix this incompatibility.  It's 
probably worth doing here given that it's relatively straightforward.

The issue with the i128 datalayout ugrade isn't the datalayout string itself, 
it's that the datalayout change affects existing code in a non-trivial way.  
That isn't really an issue here: there shouldn't be any existing AArch64 IR 
using address-space 270, so changing its meaning won't have any effect.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158857/new/

https://reviews.llvm.org/D158857

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 552917.
efriedma added a comment.

Update to address issues found so far.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

Files:
  clang/lib/CodeGen/CGCXX.cpp
  llvm/include/llvm/IR/CallingConv.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.h
  llvm/lib/Target/AArch64/AArch64CallingConvention.td
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/lib/Target/AArch64/CMakeLists.txt
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
  llvm/test/CodeGen/AArch64/arm64ec-dllimport.ll
  llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
  llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
  llvm/test/CodeGen/AArch64/stack-protector-target.ll
  llvm/test/CodeGen/AArch64/win-alloca.ll

Index: llvm/test/CodeGen/AArch64/win-alloca.ll
===
--- llvm/test/CodeGen/AArch64/win-alloca.ll
+++ llvm/test/CodeGen/AArch64/win-alloca.ll
@@ -21,4 +21,4 @@
 ; CHECK-OPT: sub [[REG3:x[0-9]+]], sp, x15, lsl #4
 ; CHECK-OPT: mov sp, [[REG3]]
 ; CHECK: bl func2
-; CHECK-ARM64EC: bl __chkstk_arm64ec
+; CHECK-ARM64EC: bl "#__chkstk_arm64ec"
Index: llvm/test/CodeGen/AArch64/stack-protector-target.ll
===
--- llvm/test/CodeGen/AArch64/stack-protector-target.ll
+++ llvm/test/CodeGen/AArch64/stack-protector-target.ll
@@ -39,6 +39,6 @@
 ; WINDOWS-ARM64EC: adrp x8, __security_cookie
 ; WINDOWS-ARM64EC: ldr x8, [x8, :lo12:__security_cookie]
 ; WINDOWS-ARM64EC: str x8, [sp, #8]
-; WINDOWS-ARM64EC: bl  _Z7CapturePi
+; WINDOWS-ARM64EC: bl "#_Z7CapturePi"
 ; WINDOWS-ARM64EC: ldr x0, [sp, #8]
-; WINDOWS-ARM64EC: bl  __security_check_cookie_arm64ec
+; WINDOWS-ARM64EC: bl "#__security_check_cookie_arm64ec"
Index: llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
 
 define void @varargs_callee(double %x, ...) nounwind {
 ; CHECK-LABEL: varargs_callee:
@@ -35,16 +35,20 @@
 ; CHECK-NEXT:sub sp, sp, #48
 ; CHECK-NEXT:mov x4, sp
 ; CHECK-NEXT:add x8, sp, #16
-; CHECK-NEXT:mov x9, #4617315517961601024
-; CHECK-NEXT:mov x0, #4607182418800017408
-; CHECK-NEXT:mov w1, #2
-; CHECK-NEXT:mov x2, #4613937818241073152
-; CHECK-NEXT:mov w3, #4
-; CHECK-NEXT:mov w5, #16
+; CHECK-NEXT:mov x9, #4617315517961601024 // =0x4014
+; CHECK-NEXT:mov x0, #4607182418800017408 // =0x3ff0
+; CHECK-NEXT:mov w1, #2 // =0x2
+; CHECK-NEXT:mov x2, #4613937818241073152 // =0x4008
+; CHECK-NEXT:mov w3, #4 // =0x4
+; CHECK-NEXT:mov w5, #16 // =0x10
 ; CHECK-NEXT:stp xzr, x30, [sp, #24] // 8-byte Folded Spill
 ; CHECK-NEXT:stp x8, xzr, [sp, #8]
 ; CHECK-NEXT:str x9, [sp]
-; CHECK-NEXT:bl varargs_callee
+; CHECK-NEXT:.weak_anti_dep varargs_callee
+; CHECK-NEXT:  .set varargs_callee, "#varargs_callee"@WEAKREF
+; CHECK-NEXT:.weak_anti_dep "#varargs_callee"
+; CHECK-NEXT:  .set "#varargs_callee", varargs_callee@WEAKREF
+; CHECK-NEXT:bl "#varargs_callee"
 ; CHECK-NEXT:ldr x30, [sp, #32] // 8-byte Folded Reload
 ; CHECK-NEXT:add sp, sp, #48
 ; CHECK-NEXT:ret
@@ -71,17 +75,21 @@
 ; CHECK-NEXT:sub sp, sp, #64
 ; CHECK-NEXT:movi v0.2d, #
 ; CHECK-NEXT:mov x4, sp
-; CHECK-NEXT:mov x8, #4618441417868443648
+; CHECK-NEXT:mov x8, #4618441417868443648 // =0x4018
 ; CHECK-NEXT:add x9, sp, #16
 ; CHECK-NEXT:add x3, sp, #32
-; CHECK-NEXT:mov x0, #4607182418800017408
-; CHECK-NEXT:mov x1, 

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: llvm/lib/Target/AArch64/AArch64CallingConvention.td:555-557
+def CSR_Win_AArch64_Arm64EC_Thunk : CalleeSavedRegs<(add X19, X20, X21, X22, 
X23, X24,
+ X25, X26, X27, X28, 
FP, LR,
+ (sequence "Q%u", 6, 
15))>;

dpaoliello wrote:
> I've been hitting asserts in `computeCalleeSaveRegisterPairs` due to this: 
> LLVM doesn't support gaps when saving registers for Windows (~line 2744 of 
> AArch64FrameLowering.cpp), so placing the smaller registers before the larger 
> ones causes issues if those smaller ones aren't paired (the `assert(OffsetPre 
> % Scale == 0);` fires since `OffsetPre` will be a multiple of 8 but `Scale` 
> will be 16).
That looks right; I thought I had that fix in here, but I guess I mixed up my 
patches and pulled in a different version.

(I'll reply to the other review comments in more detail soon.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158267: [clang][CodeGen] Avoid emitting unnecessary memcpy of records without content

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This looks sort of similar to CodeGen::isEmptyRecord... but I guess it's not 
quite the same thing.  (Even if a struct isn't "empty" in the ABI sense, there 
still might not be anything to copy.)

Instead of looking for zero-length bitfields, you probably want to just check 
isUnnamedBitfield()?  Unnamed bitfields are just padding, whether or not 
they're zero-length.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158267/new/

https://reviews.llvm.org/D158267

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: llvm/lib/Target/AArch64/AArch64MCInstLower.cpp:51-54
+// For ARM64EC, symbol lookup in the MSVC linker has limited awareness
+// of ARM64EC mangling ("#"/"$$h"). So object files need to refer to both
+// the mangled and unmangled names of ARM64EC symbols, even if they aren't
+// actually used by any relocations. Emit the necessary references here.

jacek wrote:
> I think that mangled weak symbol should link to another kind exit thunk, not 
> to unmangled symbol directly.
> 
> When an extern code symbol is resolved by an unmangled name, it means that we 
> have ARM64EC->X64 call and we need to use an exit thunk. Linker doesn't need 
> a special logic for that: on MSVC it seems to be achieved by pointing the 
> antidependency symbol to yet another exit thunk. That other exit thunk loads 
> the target exit thunk and X64 (unmangled) symbol into x10, x11 and uses 
> __os_arm64x_dispatch_icall to call emulator. I guess we may worry about that 
> later, but in that case maybe it's better not to emit mangled antidependency 
> symbol at all for now?
From what I recall, I wrote the code here primarily to deal with issues trying 
to take the address of a function; even if the function is defined in arm64ec 
code, the address is the unmangled symbol.  So there's some weirdness there 
involving symbol lookup even before there's any actual x64 code involved.

If you have a better idea of how external symbol references are supposed to be 
emitted, I'd appreciate a brief writeup, since all the information I have comes 
from trying to read dumpbin dumps of MSVC output.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157547/new/

https://reviews.llvm.org/D157547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156891: [CodeGen] Remove Constant arguments from linkage functions, NFCI.

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156891/new/

https://reviews.llvm.org/D156891

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The test changes look good to me.




Comment at: flang/lib/Decimal/CMakeLists.txt:52
 
-add_flang_library(FortranDecimal INSTALL_WITH_TOOLCHAIN
-  binary-to-decimal.cpp
-  decimal-to-binary.cpp
-)
+add_compile_options(-fPIC)
+

This `add_compile_options(-fPIC)` shouldn't be necessary?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I was probably thinking of that, yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126864/new/

https://reviews.llvm.org/D126864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157332/new/

https://reviews.llvm.org/D157332

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156821/new/

https://reviews.llvm.org/D156821

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

RecordDecl::hasFlexibleArrayMember() is supposed to reflect the standard's 
definition of a flexible array member, which only includes incomplete arrays.  
The places that care about other array members use separate checks.  We 
wouldn't want to accidentally extend the non-standard treatment of arrays with 
fixed bound to other places.

I think there was an effort to try to refactor uses of StrictFlexArrays to a 
centralized helper, but I don't remember what happened to it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126864/new/

https://reviews.llvm.org/D126864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/test/Headers/stddef.c:20-23
+// rsize_t will only be defined if __STDC_WANT_LIB_EXT1__ is set to >= 1.
+// It would be nice to test the default undefined behavior, but that emits
+// a note coming from stddef.h "rsize_t, did you mean size_t?" that can't be
+// `c99-note`d away.

iana wrote:
> Does anyone know a trick to catch the note from stddef.h?
I think the syntax is something like `expected-warning@stddef.h:10`?  See 
rGfcc699ae ,  There's also a wildcard syntax; see rG05eedf1f.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157757/new/

https://reviews.llvm.org/D157757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157757/new/

https://reviews.llvm.org/D157757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Headers/stddef.h:112
 #if defined(_MSC_EXTENSIONS) && defined(_NATIVE_NULLPTR_SUPPORTED)
-namespace std { typedef decltype(nullptr) nullptr_t; }
-using ::std::nullptr_t;
+// __need_NULL would previously define nullptr_t for C++, add it here.
+#define __need_nullptr_t

I think this code was pulled into the `__need_NULL` conditional by accident; I 
doubt anyone actually depends on the precise interaction here.  Just move to 
the same place as the other `__need_` defines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157757/new/

https://reviews.llvm.org/D157757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This approach looks fine.




Comment at: clang/lib/CodeGen/CGCall.cpp:5781
 
   // If the value is offset in memory, apply the offset now.
+  if (!isEmptyRecord(getContext(), RetTy, true)) {

The isEmptyRecord call could use a comment briefly explaining that empty 
records can overlap with other data.

The existing comment about the offset probably belongs inside the if statement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157332/new/

https://reviews.llvm.org/D157332

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697
+bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin);
+bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow);
+

artem wrote:
> efriedma wrote:
> > Putting the behavior under both "builtin" and "signed-integer-overflow" 
> > feels a little weird; is there any precedent for that?
> The problem is, we are instrumenting a builtin function, so on the one hand 
> it is reasonable to be controlled by `-fsanitize=builtin`. On the other hand, 
> GCC treats abs() as an arithmetic operation, so it is being instrumented by 
> `-fsanitize=signed-integer-overflow` (`abs(INT_MIN)` causes a negation 
> overflow).
> 
> I have decided to enable instrumentation under any of the flags, but I am not 
> sure whether it is the right choice.
I'd prefer to just follow gcc here, I think, if there isn't any strong reason 
to pick a different approach.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2702
+case LangOptions::SOB_Defined:
+  Result = Builder.CreateBinaryIntrinsic(
+  Intrinsic::abs, EmitScalarExpr(E->getArg(0)), Builder.getFalse(),

artem wrote:
> efriedma wrote:
> > Can we land the change to directly generate calls to llvm.abs() in the 
> > default case separately? This might end up impacting generated code for a 
> > variety of workloads, and I'd prefer to have a more clear bisect point.
> I used llvm.abs() for code simplicity, since middle-end combines the 
> instructions to it anyways. I think this part can be dropped entirely because 
> the intrinsic is not the best possible option either (the compiler emits 
> conditional move and fails to elide extra sign checks if the argument is 
> known to be non-negative).
Sure, that works too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156821/new/

https://reviews.llvm.org/D156821

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

You can't, in general, check whether a type is stored in a no_unique_address 
field.  Consider the following; the bad store is in the constructor S2, but the 
relevant no_unique_address attribute doesn't even show up until the definition 
of S3.

  struct S {};
  S f();
  struct S2 : public S { S2();};
  S2::S2() : S(f()) {}
  struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
  S3::S3() : x(1), y() {}

So we have to suppress all stores of empty types, whether or not we see a 
no_unique_address attribute.

Given that, I don't think the isDummy field is necessary; the important part is 
just whether the type is empty, and you can derive that directly from the type 
itself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157332/new/

https://reviews.llvm.org/D157332

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Sure, diverging from MSVC here seems fine.  I agree it's unlikely anyone would 
actually want to put a variable that will be modified in a "const" section.




Comment at: clang/lib/Sema/SemaDecl.cpp:14254
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)

dblaikie wrote:
> efriedma wrote:
> > dblaikie wrote:
> > > rnk wrote:
> > > > rsmith wrote:
> > > > > efriedma wrote:
> > > > > > rnk wrote:
> > > > > > > I think this is not compatible with MSVC. MSVC uses simple logic, 
> > > > > > > it doesn't look for mutable: https://gcc.godbolt.org/z/sj6d4saxx
> > > > > > > 
> > > > > > > The const mutable struct appears in the myrdata section in that 
> > > > > > > example.
> > > > > > > 
> > > > > > > I think the solution is to separate the flag logic from the 
> > > > > > > pragma stack selection logic, which has to remain MSVC-compatible.
> > > > > > MSVC apparently looks at whether the variable is marked "const", 
> > > > > > and nothing else; it doesn't look at mutable, it doesn't look at 
> > > > > > whether the variable has a constant initializer.  So the current 
> > > > > > code isn't right either; if we're trying to implement 
> > > > > > MSVC-compatible logic, we shouldn't check HasConstInit.
> > > > > > 
> > > > > > That said, I'm not sure how precisely/in what modes we want to 
> > > > > > precisely emulate MSVC.  Probably anything we do here will be 
> > > > > > confusing.
> > > > > We should at least issue a warning if the sensible logic and the 
> > > > > MSVC-compatible calculation differ. @rnk, do you know how important 
> > > > > it is to follow the MSVC semantics in this regard?
> > > > I think it depends on whether you think that users are primarily using 
> > > > these pragmas to override the default rdata/bss/data sections without 
> > > > any care about precisely what goes where, or if they are using them to 
> > > > do something finer grained.
> > > > 
> > > > If I had to guess, I'd say it's more likely the former, given that 
> > > > `__declspec(allocate)` and `#pragma(section)` exist to handle cases 
> > > > where users are putting specific globals into specific sections.
> > > > 
> > > > Which, if we follow Richard's suggestion, would mean warning when we 
> > > > put a global marked `const` into a writable section when 
> > > > `ConstSegStack` is non-empty. That seems reasonable. 
> > > > `-Wmicrosoft-const-seg` for the new warning group?
> > > Does the MSVC situation only apply to custom sections? (presumably when 
> > > not customizing the section, MSVC gets it right and can support a const 
> > > global with a runtime initializer, mutable member, or mutating dtor?)
> > > 
> > > I think this code still needs to be modified, since this is the code that 
> > > drives the error about incompatible sections. So it'll need to behave 
> > > differently depending on the target platform?
> > Yes, the MSVC situation is specifically if you specify `#pragma const_seg`; 
> > without the pragma, it does what you'd expect.
> Went with the "let's do the thing that the user probably wants, but isn't 
> what MSVC does, and warn when that difference comes up" - if that's OK with 
> everyone.
> 
> (always open to wordsmithing the warning - and if we want to, can go to the 
> extra layer and specifically diagnose which reason (mutable members, 
> non-const init) - and I can't quite figure out the best phrasing to say 
> "we're putting it in section X insetad of section Y, because Z, but Microsoft 
> would use X because A" or something... it's all a bit of a mouthful)
Describing which reason actually applies would make the warning a lot easier to 
read.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14254
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)

dblaikie wrote:
> rnk wrote:
> > rsmith wrote:
> > > efriedma wrote:
> > > > rnk wrote:
> > > > > I think this is not compatible with MSVC. MSVC uses simple logic, it 
> > > > > doesn't look for mutable: https://gcc.godbolt.org/z/sj6d4saxx
> > > > > 
> > > > > The const mutable struct appears in the myrdata section in that 
> > > > > example.
> > > > > 
> > > > > I think the solution is to separate the flag logic from the pragma 
> > > > > stack selection logic, which has to remain MSVC-compatible.
> > > > MSVC apparently looks at whether the variable is marked "const", and 
> > > > nothing else; it doesn't look at mutable, it doesn't look at whether 
> > > > the variable has a constant initializer.  So the current code isn't 
> > > > right either; if we're trying to implement MSVC-compatible logic, we 
> > > > shouldn't check HasConstInit.
> > > > 
> > > > That said, I'm not sure how precisely/in what modes we want to 
> > > > precisely emulate MSVC.  Probably anything we do here will be confusing.
> > > We should at least issue a warning if the sensible logic and the 
> > > MSVC-compatible calculation differ. @rnk, do you know how important it is 
> > > to follow the MSVC semantics in this regard?
> > I think it depends on whether you think that users are primarily using 
> > these pragmas to override the default rdata/bss/data sections without any 
> > care about precisely what goes where, or if they are using them to do 
> > something finer grained.
> > 
> > If I had to guess, I'd say it's more likely the former, given that 
> > `__declspec(allocate)` and `#pragma(section)` exist to handle cases where 
> > users are putting specific globals into specific sections.
> > 
> > Which, if we follow Richard's suggestion, would mean warning when we put a 
> > global marked `const` into a writable section when `ConstSegStack` is 
> > non-empty. That seems reasonable. `-Wmicrosoft-const-seg` for the new 
> > warning group?
> Does the MSVC situation only apply to custom sections? (presumably when not 
> customizing the section, MSVC gets it right and can support a const global 
> with a runtime initializer, mutable member, or mutating dtor?)
> 
> I think this code still needs to be modified, since this is the code that 
> drives the error about incompatible sections. So it'll need to behave 
> differently depending on the target platform?
Yes, the MSVC situation is specifically if you specify `#pragma const_seg`; 
without the pragma, it does what you'd expect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma created this revision.
efriedma added reviewers: dpaoliello, mstorsjo, bcl5980, jacek.
Herald added subscribers: zzheng, hiraditya, kristof.beyls.
Herald added a project: All.
efriedma requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

Arm64EC entry/exit thunks, consolidated.

This combines the previously posted patches with some additional work I've done 
to more closely match MSVC output.

Most of the important logic here is implemented in AArch64Arm64ECCallLowering.  
The purpose of the AArch64Arm64ECCallLowering is to take "normal" IR we'd 
generate for other targets, and generate most of the Arm64EC-specific bits: 
generating thunks, mangling symbols, generating aliases, and generating the 
`.hybmp$x` table.  This is all done late for a few reasons: to consolidate the 
logic as much as possible, and to ensure the IR exposed to optimization passes 
doesn't contain complex arm64ec-specific constructs.

The other changes are supporting changes, to handle the new constructs 
generated by that pass.

There's a global llvm.arm64ec.symbolmap representing the .hybmp$x entries for 
the thunks.  This gets handled directly by the AsmPrinter because it needs 
symbol indexes that aren't available before that.

There are two new calling conventions used to represent calls to and from 
thunks: ARM64EC_Thunk_X64 and ARM64EC_Thunk_Native. There are a few changes to 
handle the associated exception-handling info, SEH_SaveAnyRegQP and 
SEH_SaveAnyRegQPX.

I've intentionally left out handling for structs with small non-power-of-two 
sizes, because that's easily separated out. The rest of my current work is 
here. I squashed my current patches because they were split in ways that didn't 
really make sense. Maybe I could split out some bits, but it's hard to 
meaningfully test most of the parts independently.

I'm planning to write more IR tests for test coverage.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157547

Files:
  clang/lib/CodeGen/CGCXX.cpp
  llvm/include/llvm/IR/CallingConv.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64.h
  llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/lib/Target/AArch64/AArch64CallingConvention.h
  llvm/lib/Target/AArch64/AArch64CallingConvention.td
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64MCInstLower.cpp
  llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
  llvm/lib/Target/AArch64/AArch64Subtarget.cpp
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/lib/Target/AArch64/CMakeLists.txt
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
  llvm/test/CodeGen/AArch64/arm64ec-dllimport.ll
  llvm/test/CodeGen/AArch64/arm64ec-reservedregs.ll
  llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
  llvm/test/CodeGen/AArch64/stack-protector-target.ll

Index: llvm/test/CodeGen/AArch64/stack-protector-target.ll
===
--- llvm/test/CodeGen/AArch64/stack-protector-target.ll
+++ llvm/test/CodeGen/AArch64/stack-protector-target.ll
@@ -39,6 +39,6 @@
 ; WINDOWS-ARM64EC: adrp x8, __security_cookie
 ; WINDOWS-ARM64EC: ldr x8, [x8, :lo12:__security_cookie]
 ; WINDOWS-ARM64EC: str x8, [sp, #8]
-; WINDOWS-ARM64EC: bl  _Z7CapturePi
+; WINDOWS-ARM64EC: bl "#_Z7CapturePi"
 ; WINDOWS-ARM64EC: ldr x0, [sp, #8]
-; WINDOWS-ARM64EC: bl  __security_check_cookie_arm64ec
+; WINDOWS-ARM64EC: bl "#__security_check_cookie_arm64ec"
Index: llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
===
--- llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
+++ llvm/test/CodeGen/AArch64/arm64ec-varargs.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s | FileCheck %s
-; RUN: llc -mtriple=arm64ec-pc-windows-msvc < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s | FileCheck %s
+; RUN: llc -mtriple=arm64ec-pc-windows-msvc -arm64ec-generate-thunks=false < %s -global-isel=1 -global-isel-abort=0 | FileCheck %s
 
 define void @varargs_callee(double %x, ...) nounwind {
 ; CHECK-LABEL: varargs_callee:
@@ -35,16 +35,20 @@
 ; CHECK-NEXT:sub sp, sp, #48
 ; CHECK-NEXT:mov x4, sp
 ; CHECK-NEXT:add x8, sp, #16
-; CHECK-NEXT:mov x9, #4617315517961601024
-; CHECK-NEXT:mov x0, #4607182418800017408
-; CHECK-NEXT:mov w1, #2
-; CHECK-NEXT:mov x2, #4613937818241073152
-; CHECK-NEXT:mov w3, #4
-; CHECK-NEXT:mov w5, #16
+; 

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The overall approach here seems reasonable.  I mean, technically the undefined 
behavior is happening in the library, but detecting it early seems like a good 
idea.

This approach does have a significant limitation, though: CGBuiltin won't 
detect cases that involve taking the address of abs().  So ubsan won't end up 
picking up undefined behavior in such calls, and -fwrapv won't apply.  Maybe 
that's rare enough we don't really care, though.

Needs a release note.

Do we want to update ubsan documentation to reflect this?




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697
+bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin);
+bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow);
+

Putting the behavior under both "builtin" and "signed-integer-overflow" feels a 
little weird; is there any precedent for that?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2702
+case LangOptions::SOB_Defined:
+  Result = Builder.CreateBinaryIntrinsic(
+  Intrinsic::abs, EmitScalarExpr(E->getArg(0)), Builder.getFalse(),

Can we land the change to directly generate calls to llvm.abs() in the default 
case separately? This might end up impacting generated code for a variety of 
workloads, and I'd prefer to have a more clear bisect point.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156821/new/

https://reviews.llvm.org/D156821

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM (but please don't merge until we reach consensus on the overall feature)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155850/new/

https://reviews.llvm.org/D155850

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I see what you're getting at here... but I don't think this works quite right.  
If the empty class has a non-trivial constructor, we have to pass the correct 
"this" address to that constructor.  Usually a constructor for an empty class 
won't do anything with that address, but it could theoretically do something 
with it.

In order to preserve the address in the cases we need it, we need a different 
invariant: the handling for each aggregate expression in EmitAggExpr needs to 
ensure it doesn't store anything to an empty class.  Which is unfortunately 
more fragile than I'd like, but I can't think of a better approach.  This check 
needs to happen further down the call stack.  Maybe put it in 
CodeGenFunction::EmitCall.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157332/new/

https://reviews.llvm.org/D157332

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156466: [clang][CGExprConstant] handle implicit widening/narrowing Int-to-Int casts

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156466/new/

https://reviews.llvm.org/D156466

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156378: [clang][CGExprConstant] handle unary negation on integrals

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156378/new/

https://reviews.llvm.org/D156378

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I'm happy with this approach.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4327
+if (D && D->hasAttr() && isa(Entry))
+  DeferredAnnotations[cast(Entry)] = cast(D);
+

This doesn't quite seem sufficient... I'd expect you'd want to update the map 
when you see a new declaration.  Otherwise we miss annotations on something 
like:

```
void foo(void);
void *xxx = (void*)foo;
void __attribute__((annotate("bar"))) foo();
```

Granted, I wouldn't expect this sort of construct to show up normally, and I'm 
not sure how hard this is to fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156172/new/

https://reviews.llvm.org/D156172

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5559
+return EmitStdParUnsupportedBuiltin(this, FD);
+  else
+ErrorUnsupported(E, "builtin function");

Else-after-return.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5315
+  !GV->isThreadLocal() &&
+  !(getLangOpts().HIPStdPar && getLangOpts().CUDAIsDevice)) {
 if (D->getTLSKind() == VarDecl::TLS_Dynamic)

You can't just pretend a thread-local variable isn't thread-local.  If the 
intent here is that thread-local variables are illegal in device code, you need 
to figure out some way to produce a diagnostic.  (Maybe by generating a call to 
__stdpar_unsupported_threadlocal or something like that if code tries to refer 
to such a variable.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155850/new/

https://reviews.llvm.org/D155850

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156482: [clang][CGExprConstant] handle FunctionToPointerDecay

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1128
+case CK_NoOp:
+case CK_NonAtomicToAtomic:
   return Visit(subExpr, destType);

I think I'd prefer to continue treating an undecayed function as an "lvalue", 
to keep things straightforward.  To that end, I'd prefer a separate 
"ConstLValueExprEmitter", or something like that, so ConstExprEmitter only 
visits rvalues.

The new emitter should reuse code from ConstLValueExprEmitter where possible.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1247
+if (isa(VD))
+  return CGM.getModule().getNamedValue(VD->getName());
+return nullptr;

You 100% can't use `VD->getName()` like this; I'm shocked this isn't causing 
any test failures.  GetAddrOfFunction() resolves names correctly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156482/new/

https://reviews.llvm.org/D156482

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156466: [clang][CGExprConstant] handle implicit widening/narrowing Int-to-Int casts

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1152
+  return CI;
+llvm::APInt A = CI->getValue().sextOrTrunc(DstWidth);
+return llvm::ConstantInt::get(CGM.getLLVMContext(), A);

sextOrTrunc() is, in general, wrong; if FromType is unsigned, you need 
zextOrTrunc.  (This works transparently in HandleIntToIntCast because it's 
using APSInt, which tracks the sign bit of the operand.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156466/new/

https://reviews.llvm.org/D156466

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156378: [clang][CGExprConstant] handle unary negation on integrals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1366
 
+  llvm::Constant *VisitUnaryOperator(UnaryOperator *U, QualType T) {
+switch (U->getOpcode()) {

StmtVisitor supports defining a function "VisitUnaryMinus", so you don't have 
to switch() over the opcode.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156378/new/

https://reviews.llvm.org/D156378

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

In D76096#4540442 , @nickdesaulniers 
wrote:

> Consider the following code:
>
>   long x [] = {1, 2, 3, 4, 5 + 4};
>
> Even with some of my recent changes, because the fast path parallel 
> implementation doesn't currently handle BinaryOperator Exprs, we'll visit all 
> of the ImplicitCastExpr and IntegerLiteral, then eventually figure out that 
> we can't constant evaluate the final element (for the wrong reasons; i.e. 
> VisitBinaryOperator not implemented).  That's a slow down because then we'll 
> fall back to EvaluateAsLValue/EvaluateAsRValue which will then succeed.
>
> So we pretty much need a fair amount of parallel implementation for the fast 
> path to succeed.  I'm happy to implement all that, if that's the direction 
> you're advising?
>
> Or did you mean something else when you said "for structs and arrays?"

The given example doesn't fallback in the "bad" way: we don't create an APValue 
representing `x[]`.  If you look at the code for lowering InitListExpr, you'll 
note that it doesn't actually just recursively Visit() like we do in other 
places.  We do create an lvalue for "5+4", but that isn't expensive in the same 
way.

> Is there another way I can prove the absence of regression to you?

At this point, I'm basically convinced the regressions are only going to be in 
obscure cases we aren't going to find by benchmarking.  I'm sure someone will 
find them eventually, but we can deal with them as they come up, I think.

So LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14254
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)

rnk wrote:
> I think this is not compatible with MSVC. MSVC uses simple logic, it doesn't 
> look for mutable: https://gcc.godbolt.org/z/sj6d4saxx
> 
> The const mutable struct appears in the myrdata section in that example.
> 
> I think the solution is to separate the flag logic from the pragma stack 
> selection logic, which has to remain MSVC-compatible.
MSVC apparently looks at whether the variable is marked "const", and nothing 
else; it doesn't look at mutable, it doesn't look at whether the variable has a 
constant initializer.  So the current code isn't right either; if we're trying 
to implement MSVC-compatible logic, we shouldn't check HasConstInit.

That said, I'm not sure how precisely/in what modes we want to precisely 
emulate MSVC.  Probably anything we do here will be confusing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

That's scary: it means we can silently behave differently from other compilers 
in a way that's hard to understand.  Is there some way we can provide a 
diagnostic?

That said, it looks like that's an existing issue, so I'm fine with addressing 
it separately.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155955/new/

https://reviews.llvm.org/D155955

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The general approach seems fine.  The multiplier for constexpr vs. constant 
folding can be left for a followup, and we can continue to consider other 
possible improvements elsewhere.

I guess I have one remaining question here: how does this interact with SFINAE? 
 In other words, if we hit the limit analyzing the signature of a function, do 
we print an error, or silently remove the function from the overload list?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155955/new/

https://reviews.llvm.org/D155955

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> But prior to D151587 , we did that for C++. 
> Why is C special here?  And prior to this patch, we did that for C++ 11+. Why 
> is C++ 03 special here?

I'm trying to avoid regressions.

C++11 made constant evaluation a lot more complicated, so in C++11 we 
evaluate() every global to determine const-ness.  But it's always worked that 
way, so there's no regression.  I'm not exactly happy C++11 made everything 
more expensive, but fixing that is a lot harder, and not a regression.

> So I feel like your feedback is pulling me in opposite directions. You want 
> to avoid the fast path falling back to the slow path, but improvements to the 
> fast path directly result in "complete parallel infrastructure" which you 
> also don't want. Those seem mutually exclusive to me. Is there a third 
> option? Am I attacking a strawman? (Regardless, I don't want to seem 
> unappreciative of the reviews, advice, or discussion).

The primary thing that makes Evaluate/APValue slow is the fact that it has a 
very inefficient representation of structs and arrays.  Using it to evaluate 
simple integer expressions is largely comparable to non-Evaluate() methods.  
The idea is to maintain the parallel infrastructure for structs and arrays, but 
not for other things.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156175: [clang][ConstExprEmitter] handle NullToPointer ImplicitCastExpr

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156175/new/

https://reviews.llvm.org/D156175

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156274: [NFC] Avoid potential dereferencing of nullptr.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:1659
   mangleCXXDtorType(Dtor_Complete);
+assert(ND);
 writeAbiTags(ND, AdditionalAbiTags);

aaron.ballman wrote:
> This seems incorrect -- if the declaration name is a destructor, then `ND` 
> should always be null, right?
Seems okay to me?  ND is the destructor declaration.



Comment at: clang/lib/CodeGen/CGObjCMac.cpp:1806
 // Complex types.
+assert(contBB);
 CGF.EmitBlock(contBB);

aaron.ballman wrote:
> Hmmm, is assert the correct approach here? It seems to me that a Complex 
> rvalue will trigger this assertion. I think this deserves some additional 
> thinking rather than silencing the static analysis warning. CC @efriedma 
> @rjmccall who might have ideas.
Yes, if a complex value hit this codepath with a null contBB, it would just 
crash, so the analysis is flagging a real issue.

I'm not sure how to write ObjC code to actually trigger this codepath, though.  
Probably involves a `_Complex _BitInt(128)` or something like that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156274/new/

https://reviews.llvm.org/D156274

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154911: Enabling fstack_clash_protection for arm32 bit, thumb and thumb2 mode

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3460
+  } else if (EffectiveTriple.isArm() || EffectiveTriple.isThumb()) {
+CmdArgs.push_back("-mstack-probe-size=1024");
+  }

tnfchris wrote:
> efriedma wrote:
> > Why 1024?
> 1024 was experimentally determined by Arm and is part of the ABI for stack 
> clash (which has not yet been published).  It was determined by examining a 
> large number of programs and looking at the function stack usages.  1024 
> covers 80-90% of programs such that we can minimize the number of probes 
> required in the average cases. 
There are actually multiple numbers involved here, no?  One is the spacing of 
probes, i.e. if allocating a large amount of stack, how many times you need to 
probe; this is basically the page size of the target. the other is how much 
unprobed space a function is allowed to allocate before calling another 
function. Referring to the the AArch64 patch, -mstack-probe-size is the former, 
the hardcoded "1024" is the latter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154911/new/

https://reviews.llvm.org/D154911

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: flang/runtime/CMakeLists.txt:251
 
-  INSTALL_WITH_TOOLCHAIN
-)
+if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)
+  add_flang_library(FortranRuntime STATIC

klausler wrote:
> pscoro wrote:
> > efriedma wrote:
> > > pscoro wrote:
> > > > efriedma wrote:
> > > > > pscoro wrote:
> > > > > > efriedma wrote:
> > > > > > > This "if" doesn't make sense to me.  If we're not building 
> > > > > > > flang-rt, we shouldn't be here, so I don't see why you need an 
> > > > > > > "if" in the first place.
> > > > > > `add_subdirectory(runtime)` is a line that still exists in 
> > > > > > `flang/CMakeLists.txt`. This exists because `Fortran_main` is still 
> > > > > > being built at the same time as the compiler, and to do so, the 
> > > > > > runtime subdirectory still needs to be added to flang 
> > > > > > (`flang/CMakeLists.txt` -> `add_subdirectory(runtime)` -> 
> > > > > > `flang/runtime/CMakeLists.txt` -> `add_subdirectory(FortranMain)`. 
> > > > > > The solution I had was to just add a check around the 
> > > > > > `FortranRuntime` library production so that it only happens for 
> > > > > > flang-rt.
> > > > > > 
> > > > > > If you have a better solution let me know. Overall, I'm not sure if 
> > > > > > Fortran_main is currently being handled in the best way (ie, its 
> > > > > > still being built at the same time as the compiler, which doesn't 
> > > > > > seem ideal), but am not sure what course of action to take with it 
> > > > > > since it doesn't really belong in flang-rt either (see 
> > > > > > documentation for details)
> > > > > Fortran_main should be "part of" flang-rt in the sense that building 
> > > > > flang-rt builds it.  Most of the same reasons we want to build 
> > > > > flang-rt.a as a runtime apply.
> > > > > 
> > > > > Since the output needs to be separate, building flang-rt should 
> > > > > produce two libraries: flang-rt.a and FortranMain.a.
> > > > I agree with this idea and have been trying to make it work but in the 
> > > > process I discovered that my "fix" above (`if (DEFINED 
> > > > LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)`) is 
> > > > worse than I thought.
> > > > 
> > > > By building the llvm target with flang-rt as an enabled runtime, we are 
> > > > essentially saying: build the flang compiler, and then invoke cmake 
> > > > again to build the runtimes project (externally), which builds flang-rt.
> > > > 
> > > > The problem is that check-all depends on check-flang which depends on 
> > > > the runtime. The if guard above was not actually doing what I thought 
> > > > it was, and the reason why configuring didnt fail with "flang-rt does 
> > > > not exist" is because the if guard was still evaluating to true. 
> > > > Basically, the guard ensured that FortranRuntime was only being built 
> > > > if flang-rt is built, but the add_library call was still being reached 
> > > > *during the configuration of the flang compiler*.
> > > > 
> > > > I am having trouble dealing with this dependency since runtimes get 
> > > > built externally (and after the compiler is built, which is when the 
> > > > check-all custom target gets built). I will have to investigate more 
> > > > closely how other runtimes handle this. My initial thought was perhaps 
> > > > the runtime testing should be decoupled from check-flang/check-all 
> > > > however I don't know if that's a good idea, and I also think that only 
> > > > helps with flang-rt, but if Fortran_main is required to build any 
> > > > executable I imagine that it should be needed for check-flang?
> > > > 
> > > > This is just an update of whats holding me back right now. If you have 
> > > > any tips please let me know. Thanks for bringing this to my attention.
> > > For comparison, check-clang doesn't require any runtime libraries at all. 
> > > All the checks only check the generated LLVM IR/driver commands/etc.  Any 
> > > tests that require runtime execution go into either the regression tests 
> > > for the runtimes themselves, or into lllvm-test-suite.
> > > 
> > > Probably check-flang should do something similar. It probably makes sense 
> > > to add a check-flang-rt target or something like that.  (Not sure which 
> > > existing flang tests are impacted.)
> > If we are decoupling the affected tests from flang-rt, then I think the 
> > only component left coupled (that I'd argue shouldn't be coupled) is the 
> > runtime source files. I'm seeing that for all the other runtimes in the 
> > llvm-project, the sources exist in a top level directory. Would it make 
> > sense and be feasible for flang-rt to act similarly?
> > 
> > Comparing to the infrastructure for compiler-rt, I've been entertaining the 
> > idea that FortranRuntime and FortranMain can be treated as a library with 
> > sources defined in `flang-rt/lib/FortranRuntime` and 
> > `flang-rt/lib/FortranMain`, similar to how compiler-rt has 

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Basically, I don't want order-of-magnitude compile-time regressions with large 
global variables.  There are basically two components to that:

- That the fast path for emitting globals in C continues be fast.
- That we rarely end up using evaluateValue() on large global arrays/structs in 
C, for reasons other than emitting them.  If we end up calling evaluateValue() 
on most globals anyway, the codegen fast-path is a lot less useful; most of the 
cost of the APValue codepath is generating the APValue, not lowering it.

This is not quite as concrete as I'd like, but I'm not sure how to describe it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

I don't have any concern with this specific patch; LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156185/new/

https://reviews.llvm.org/D156185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156175: [clang][ConstExprEmitter] handle NullToPointer ImplicitCastExpr

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1131-1132
+case CK_NullToPointer: {
+  if (llvm::Constant *C = Visit(subExpr, destType))
+if (C->isNullValue())
+  return CGM.EmitNullConstant(destType);

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > FWIW, I would have thought these might be unnecessary, but we do trip 2 
> > > tests without these guards IIRC.
> > The fact that the Visit can fail doesn't surprise me; you can write 
> > arbitrary expressions of type nullptr_t (for example, `nullptr_t f(); void* 
> > g = f();`.  I can't think of any reason you'd need to check 
> > C->isNullValue(), though; do you have a testcase for that?
> Failed Tests (2):
>   Clang :: CodeGenCXX/const-init-cxx11.cpp
>   Clang :: CodeGenCXX/cxx11-thread-local-instantiated.cpp
> 
> Looking at the first:
> 
> ```
> decltype(nullptr) null();
> int *p = null();
> ```
> and the corresponding AST:
> ```
> |-FunctionDecl 0x5650ebbb44a8  col:21 used null 
> 'decltype(nullptr) ()'
> `-VarDecl 0x5650ebbb45e0  col:8 p 'int *' cinit
>   `-ImplicitCastExpr 0x5650ebbb4740  'int *' 
> `-CallExpr 0x5650ebbb4720  
> 'decltype(nullptr)':'std::nullptr_t'
>   `-ImplicitCastExpr 0x5650ebbb4708  'decltype(nullptr) (*)()' 
> 
> `-DeclRefExpr 0x5650ebbb4690  'decltype(nullptr) ()' lvalue 
> Function 0x5650ebbb44a8 'null' 'decltype(nullptr) ()'
> ```
> 
> 
> we change the existing test case from having a `__cxx_global_var_init` 
> routine, i.e.:
> ```
> @p = dso_local global ptr null, align 8
> @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, 
> ptr } { i32 65535, ptr @_GLOBAL__sub_I_x.cpp, ptr null }]
> 
> ; Function Attrs: noinline uwtable
> define internal void @__cxx_global_var_init() #0 section ".text.startup" {
> entry:
>   %call = call ptr @_Z4nullv()
>   store ptr null, ptr @p, align 8
>   ret void
> }
> 
> declare ptr @_Z4nullv() #1
> 
> ; Function Attrs: noinline uwtable
> define internal void @_GLOBAL__sub_I_x.cpp() #0 section ".text.startup" {
> entry:
>   call void @__cxx_global_var_init()
>   ret void
> }
> ```
> to simply:
> ```
> @p = dso_local global ptr null, align 8
> ```
> So it seems nice to skip having the static constructors, but then `@_Z4nullv` 
> is never invoked.  That seems problematic? (I expect the fast path to fail 
> once it recurses down to the DeclRefExpr, but that requires the subexpr of 
> the ImplicitCastExpr be visited).
> 
> ---
> 
> So I think the code as written is good to go. WDYT?
I think you need to check that Visit() succeeds (returns a non-null Constant*), 
but like I said before, I can't see why you need to check `C->isNullValue()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156175/new/

https://reviews.llvm.org/D156175

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Slightly messed up my example because I forgot the function was unprototyped.  
The following should show what I mean:

  void foo(void);
  void *xxx = (void*)foo;
  __attribute__((annotate("bar"))) void foo(){}

In terms of where the right place is, I don't recall the exact structure of 
that code off the top of my head, but I think there's somewhere we handle 
redeclarations?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156172/new/

https://reviews.llvm.org/D156172

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

No, that's taking the address of a string literal lvalue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156185/new/

https://reviews.llvm.org/D156185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The idea of emitting annotations on declarations seems fine.  (LLVM itself 
doesn't consume annotations anyway; they're meant as an extension mechanism for 
third-party tools.)

I'm a bit concerned the way this is implemented will end up dropping 
annotations we would previously emit.  For example:

  void foo();
  void *xxx = (void*)foo;
  __attribute__((annotate("bar"))) void foo(){}


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156172/new/

https://reviews.llvm.org/D156172

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> so if you were concerned with arrays of string literals, we need this patch.

Ohhh that's not what I meant by StringLiterals.  I meant cases where the 
string literal is an rvalue, like `char foo[10] = "x";`.  If it's an 
lvalue, we go through ConstantLValueEmitter, which is still reasonably fast.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156185/new/

https://reviews.llvm.org/D156185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> I guess I lost focus on the "struct or array part."

Right... scalars weren't the concern for D76096 
.  They have much less overhead going through 
ExprConstant, and any evaluation of scalars isn't a regression because we 
currently don't have a codepath for that anyway.

> Some of these seem like very low hanging fruit, IMO, and probably still do 
> speed up constant evaluation.

Sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156185/new/

https://reviews.llvm.org/D156185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156175: [clang][ConstExprEmitter] handle NullToPointer ImplicitCastExpr

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1131-1132
+case CK_NullToPointer: {
+  if (llvm::Constant *C = Visit(subExpr, destType))
+if (C->isNullValue())
+  return CGM.EmitNullConstant(destType);

nickdesaulniers wrote:
> FWIW, I would have thought these might be unnecessary, but we do trip 2 tests 
> without these guards IIRC.
The fact that the Visit can fail doesn't surprise me; you can write arbitrary 
expressions of type nullptr_t (for example, `nullptr_t f(); void* g = f();`.  I 
can't think of any reason you'd need to check C->isNullValue(), though; do you 
have a testcase for that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156175/new/

https://reviews.llvm.org/D156175

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

We can do a whole bunch of these, but at some point we hit diminishing 
returns... bailing out here isn't *that* expensive.  Do you have some idea how 
far you want to go with this?  Adding a couple obvious checks like this isn't a 
big deal, but I don't want to build up a complete parallel infrastructure for 
scalar expressions here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156185/new/

https://reviews.llvm.org/D156185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156182: [clang][CodeGenModule] remove declaration of GetAddrOfConstantString

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156182/new/

https://reviews.llvm.org/D156182

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156154: [clang][ConstExprEmitter] handle IntegerLiterals

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

That said, skipping all those abstraction layers for the simplest cases is 
probably worth it; LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156154/new/

https://reviews.llvm.org/D156154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156154: [clang][ConstExprEmitter] handle IntegerLiterals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

We don't completely fall back if a subexpression fails to evaluate.  
EmitArrayInitialization etc. don't recursively visit; they use 
tryEmitPrivateForMemory, so we can fallback for subexpressions.  
(tryEmitPrivateForMemory can still fail for certain constructs, like the 
MaterializeTemporary cases we spent so much time on, but the most common cases 
should work.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156154/new/

https://reviews.llvm.org/D156154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156154: [clang][ConstExprEmitter] handle IntegerLiterals and IntegralCasts

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

As a practical matter, I'm not sure this helps much; ExprConstant should be 
reasonably fast for simple integers.  The performance issues only really show 
for structs/arrays.  But maybe it helps a little to handle a few of the most 
common integer expressions.




Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1128
 case CK_ConstructorConversion:
+case CK_IntegralCast:
   return Visit(subExpr, destType);

This doesn't look right; I suspect this will return an integer with the wrong 
width.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156154/new/

https://reviews.llvm.org/D156154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> Note sure what you mean. Making sure we use size_t for all array extents is 
> not something that can be fixed overnight but more importantly, it does not 
> help: Would it not overflow, the allocation would still fail.

Oh, right, it would be sizeof(APValue) * UINT_MAX, which would break in any 
practical usage.

> I don't think that would make sense in actual code, and having some sort of 
> sparse array is something I considered. And we already delay allocation in 
> some cases, but we do need to create elements to destroy them, read them, 
> etc. So while it may be possible, it seems... complicated.

Definitely not simple, sure.

> I think a more viable long term fix might be to not crash on allocation 
> failure, and/or to have a way to limit the allocation to some portion of the 
> available memory.

Making the compiler's behavior depend on the amount of memory installed in the 
user's computer seems like a non-starter.  I think we just have to stick with 
some combination of:

- Try to reduce excessive memory usage in constant folding.
- Add strict limits memory usage limits for optional constant folding
- Maybe consider disobeying the standard slightly in certain cases: the 
standard requires that we constant-fold the initializers for all global 
variables, but that might not really be viable for globals that are expensive 
to evaluate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155955/new/

https://reviews.llvm.org/D155955

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The UINT_MAX thing seems like a straightforward bug; if we have time to fix it 
properly, I'd prefer not to add weird workarounds.

The release note says "unless they are part of a constant expression", but I 
don't see any code in the implementation that distinguishes folding from 
constant expression evaluation.  Unless this is just referring to the fact that 
bailing out of folding doesn't produce an error?  We might want to consider 
using a stricter bound for optional folding, though.

How likely is it that we could add some sort of optimization for new 
expressions so we don't represent each element separately in memory?  I know 
there's no solution in general, but in the cases people actually care about, 
all/almost all the elements are identical.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155955/new/

https://reviews.llvm.org/D155955

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> Is the idea for the way forward here to ensure (i.e. adding code such) that 
> ConstExprEmitter can constant evaluate such Expr's?

For that exact construct, EvaluateAsRValue will also fail, so there's no real 
regression.  The issue would be for a constant global variable... but if we try 
to handle that, we get into the exact mess of complicated code for 
lvalue-to-rvalue conversions in CGExprConstant we were trying to avoid adding.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Hmm, that kind of construct could run into issues with large global variables.  
If we "inline" a bunch of large global constants into initializers for arrays, 
we could significantly increase codesize.  Not sure how likely that is in 
practice.  We could maybe consider teaching CGExprConstant to abort in such 
cases (not sure if it currently tracks whether we're initializing a global 
variable or a local.)  Not sure what the heuristics for that would look like.

Thinking about it a bit more, maybe we should just observe overall compiler 
behavior.  If the compile-time and codesize for the Linux kernel (and maybe 
also the llvm test-suite) is mostly unchanged, we're probably fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The ones most likely to be a concern are InitListExpr and StringLiteral.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The idea would be looking for places we EvaluateAsRValue an array or struct.  
Not sure what that stack trace represents.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

My primary concern here is making sure we don't actually blow up compile-time.  
 D151587  fixes the dependency from 
CGExprConstant, which was the most complicated one, but there are other places 
that call into Evaluate().  Some of those are probably unavoidable, but I'd 
like to make sure at least that we don't end up evaluating the initializers of 
global variables in C code.

Not sure there's any good way to regression-test that, though...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/Expr.cpp:3444
 if (CE->getCastKind() == CK_NoOp ||
 CE->getCastKind() == CK_LValueToRValue ||
 CE->getCastKind() == CK_ToUnion ||

nickdesaulniers wrote:
> efriedma wrote:
> > efriedma wrote:
> > > An CK_LValueToRValue conversion needs to change IsForRef (or else you're 
> > > checking whether the address of the value is constant, not the value 
> > > itself).
> > Not sure what I was thinking when I wrote the original comment here.  We 
> > need to bail on LValueToRValue here: the only way to correctly do an 
> > lvalue-to-rvalue conversion is to evaluate the operand as an lvalue, then 
> > convert the resulting lvalue to an rvalue.  We need to use ExprConstant to 
> > do that.
> when you say `we need to bail on LValueToRValue here` do you just `return 
> false` for that case or what?
Probably just "break;".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> restore E->getStorageDuration() == SD_Static check to fix libcxx regressions

Makes sense... but it would be nice to add a testcase for whatever construct 
was triggering this.




Comment at: clang/lib/AST/Expr.cpp:3462-3468
   ->isConstantInitializer(Ctx, false, Culprit);
   case CXXDefaultArgExprClass:
 return cast(this)->getExpr()
   ->isConstantInitializer(Ctx, false, Culprit);
   case CXXDefaultInitExprClass:
 return cast(this)->getExpr()
   ->isConstantInitializer(Ctx, false, Culprit);

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > @efriedma should a few more of these cases be changed from passing 
> > > `false` as the `ForRef` param of `isConstantInitializer` to `IsForRef`?  
> > > If I change the rest of the cases except for 
> > > `MaterializeTemporaryExprClass` then tests are still green.
> > One of the reasons I didn't go with this approach is that I really didn't 
> > want to think about this question...
> > 
> > We probably don't have good test coverage for the C++ constructs, since we 
> > don't really use isConstantInitializer() outside of C++03 mode.
> It looks like I should be able to test this against all of Android, and 
> against Google's internal C++ codebase.  Let me work through that, and see if 
> I can shake out additional tests cases to add here so that we have lower risk 
> of regression.
I think I'd prefer to go back to the original way I wrote this.  I really just 
don't want to think about the right way of translating the various new kinds of 
lvalues that will be hitting this codepath.

I experimented with actually removing the relevant call to 
isConstantInitializer (basically forcing C++03 code onto the C++11 codepath for 
global vars), but it seemed sort of invasive/risky.  Maybe still worth 
considering at some point, I guess, but I don't want to mix too many things 
into this patch.

My point with the test coverage was that this primarily impacts -std=c++03, in 
a subtle way that won't be obvious in a lot of code.  I'd be surprised if you 
have significant amounts of code at Google that's still compiled with 
-std=c++03.



Comment at: clang/lib/AST/Expr.cpp:3444
 if (CE->getCastKind() == CK_NoOp ||
 CE->getCastKind() == CK_LValueToRValue ||
 CE->getCastKind() == CK_ToUnion ||

efriedma wrote:
> An CK_LValueToRValue conversion needs to change IsForRef (or else you're 
> checking whether the address of the value is constant, not the value itself).
Not sure what I was thinking when I wrote the original comment here.  We need 
to bail on LValueToRValue here: the only way to correctly do an 
lvalue-to-rvalue conversion is to evaluate the operand as an lvalue, then 
convert the resulting lvalue to an rvalue.  We need to use ExprConstant to do 
that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> In what regards how to do deferred diagnostics, it think it can be done like 
> this (I crossed streams in my prior reply when discussing this part, so it's 
> actually nonsense): instead of emitting undef here, we can emit a builtin 
> with the same signature, but with the name suffixed with e.g. 
> (`__stdpar_unsupported`) or something similar. Then, when doing the 
> reachability computation later, if we stumble upon a node in the CFG that 
> contains a builtin suffixed with `__stdpar_unsupported` we error out, and can 
> provide nice diagnostics since we'd have the call-chain handy. Thoughts?

Sure, something like that.  If you stick a SourceLocation on it, you can even 
recover the original clang source location.

> We can definitely go with the __clang_unsupported approach, but I think I'd 
> prefer these to be compile time errors rather than remarks + runtime printf, 
> not in the least because printf adds some overhead.

The overhead should be pretty minimal if the code doesn't actually run.

> So TL;DR, I think it would be more complex to do this on the AST and would 
> end up more brittle / less future proof.



> Since we need to support -O0

The biggest downside of working in the backend is that it becomes very hard for 
users to predict what will compile, and will not compile.  Particularly if you 
want to support -O0.  (I was sort of assuming you just wouldn't support -O0.)  
If you work on the AST, fewer constructs will be accepted, but you can actually 
define rules about which constructs will/will not be accepted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155850/new/

https://reviews.llvm.org/D155850

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5542
+  if (!getLangOpts().HIPStdPar)
+ErrorUnsupported(E, "builtin function");
 

AlexVlx wrote:
> efriedma wrote:
> > This doesn't make sense; we can't just ignore bits of the source code.  I 
> > guess this is related to "the decision on their validity is deferred", but 
> > I don't see how you expect this to work.
> This is one of the weirder parts, so let's consider the following example:
> 
> ```cpp
> void foo() { __builtin_ia32_pause(); }
> void bar() { __builtin_trap(); }
> 
> void baz(const vector& v) {
> return for_each(par_unseq, cbegin(v), cend(v), [](auto&& x) { if (x == 
> 42) bar(); });
> }
> ```
> 
> In the case above, what we'd offload to the accelerator, and ask the target 
> BE to lower, is the implementation of `for_each`, and `bar`, because it is 
> reachable from the latter. `foo` is not reachable by any execution path on 
> the accelerator side, however it includes a builtin that is unsupported by 
> the accelerator (unless said accelerator is x86, which is not impossible, but 
> not something we're dealing with at the moment). If we were to actually error 
> out early, in the FE, in these cases, there's almost no appeal to what is 
> being proposed, because standard headers, as well as other libraries, are 
> littered with various target specific builtins that are not going to be 
> supported. This all builds on the core invariant of this model / extension / 
> thingamabob, which is that the algorithms, and only the algorithms, are 
> targets for offload. It thus follows that as long as code that is reachable 
> from an algorithm's implementation is safe, all is fine, but we cannot know 
> this in the FE / on an AST level, because we need the actual CFG. This part 
> is handled in LLVM in the `SelectAcceleratorCodePass` that's in the last 
> patch in this series.
> 
> Now, you will quite correctly observe that there's nothing preventing an user 
> from calling `foo` in the callable they pass to an algorithm; they might read 
> the docs / appreciate that this won't work, but even there they are not safe, 
> because there via some opaque call chain they might end up touching some 
> unsupported builtin. My intuition here, which is reflected above in letting 
> builtins just flow through, is that such cases are better served with a 
> compile time error, which is what will obtain once the target BE chokes 
> trying to lower an unsupported builtin. It's not going to be a beautiful 
> error, and we could probably prettify it somewhat if we were to check after 
> we've done the accelerator code selection pass, but it will happen at compile 
> time. Another solution would be to emit these as traps (poison + trap for 
> value returning ones), but I am concerned that it would lead to really 
> fascinating debug journeys.
> 
> Having said this, if there's a better way to deal with these scenarios, it 
> would be rather nice. Similarly, if the above doesn't make sense, please let 
> me know.
> 
Oh, I see; you "optimistically" compile everything assuming it might run on the 
accelerator, then run LLVM IR optimizations, then determine late which bits of 
code will actually run on the accelerator, which then prunes the code which 
shouldn't run.

I'm not sure I really like this... would it be possible to infer which 
functions need to be run on the accelerator based on the AST?  I mean, if your 
API takes a lambda expression that runs on the accelerator, you can mark the 
lambda's body as "must be emitted for GPU", then recursively mark all the 
functions referred to by the lambda.

Emiting errors lazily from the backend means you get different diagnostics 
depending on the optimization level.

If you do go with this codegen-based approach, it's not clear to me how you 
detect that a forbidden builtin was called; if you skip the error handling, you 
just get a literal "undef".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155850/new/

https://reviews.llvm.org/D155850

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76096: [clang] allow const structs to be constant expressions for C

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/test/Sema/builtins.c:181-186
+  ASSERT(!OPT(test17_c));
+  ASSERT(!OPT(_c[0]));
+  ASSERT(!OPT((char*)test17_c));
   ASSERT(!OPT(test17_d));// expected-warning {{folding}}
   ASSERT(!OPT(_d[0]));// expected-warning {{folding}}
   ASSERT(!OPT((char*)test17_d)); // expected-warning {{folding}}

nickdesaulniers wrote:
> `test17_c` and `test17_d` are both declared as `const char [4];` not sure why 
> this case would differ
`strlen(test17_c)` is 3; `strlen(test17_d)` is undefined behavior.  I assume 
the difference is because the latter doesn't fold.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76096/new/

https://reviews.llvm.org/D76096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

[Eli Friedman via Phabricator via cfe-commits]

efriedma requested changes to this revision.
efriedma added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5542
+  if (!getLangOpts().HIPStdPar)
+ErrorUnsupported(E, "builtin function");
 

This doesn't make sense; we can't just ignore bits of the source code.  I guess 
this is related to "the decision on their validity is deferred", but I don't 
see how you expect this to work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155850/new/

https://reviews.llvm.org/D155850

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/Expr.cpp:3462-3468
   ->isConstantInitializer(Ctx, false, Culprit);
   case CXXDefaultArgExprClass:
 return cast(this)->getExpr()
   ->isConstantInitializer(Ctx, false, Culprit);
   case CXXDefaultInitExprClass:
 return cast(this)->getExpr()
   ->isConstantInitializer(Ctx, false, Culprit);

nickdesaulniers wrote:
> @efriedma should a few more of these cases be changed from passing `false` as 
> the `ForRef` param of `isConstantInitializer` to `IsForRef`?  If I change the 
> rest of the cases except for `MaterializeTemporaryExprClass` then tests are 
> still green.
One of the reasons I didn't go with this approach is that I really didn't want 
to think about this question...

We probably don't have good test coverage for the C++ constructs, since we 
don't really use isConstantInitializer() outside of C++03 mode.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This seems to be close to a good state.




Comment at: clang/lib/AST/Expr.cpp:3444
 if (CE->getCastKind() == CK_NoOp ||
 CE->getCastKind() == CK_LValueToRValue ||
 CE->getCastKind() == CK_ToUnion ||

An CK_LValueToRValue conversion needs to change IsForRef (or else you're 
checking whether the address of the value is constant, not the value itself).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> Indeed, but I wonder why only do this when IsForRef == true (narrator: it's 
> not)?

A MaterializeTemporaryExpr is an lvalue; it only makes sense in lvalue contexts.

> Also, why for MaterializeTemporaryExpr manually force the ForRef parameter to 
> false?

The operand to a MaterializeTemporaryExpr is an rvalue.

> It seems like perhaps we want to call EvaluateAsLValue in 
> Expr::isConstantInitializer AFTER checking the statement class. Otherwise 
> we're basically reimplementing that big switch statement again; at least two 
> cases as suggested by your diff.

I think we want to keep lvalue and rvalue handling separate, for the same 
reasons we want to keep it separate in other places.  It's very easy to get the 
semantics wrong if you try to handle lvalues and rvalues in the same switch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This seems to work:

  diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
  index 25d3535e..98b1e4d 100644
  --- a/clang/lib/AST/Expr.cpp
  +++ b/clang/lib/AST/Expr.cpp
  @@ -3319,6 +3319,10 @@ bool Expr::isConstantInitializer(ASTContext , bool 
IsForRef,
 // kill the second parameter.
  
 if (IsForRef) {
  +if (auto *EWC = dyn_cast(this))
  +  return EWC->getSubExpr()->isConstantInitializer(Ctx, true, Culprit);
  +if (auto *MTE = dyn_cast(this))
  +  return MTE->getSubExpr()->isConstantInitializer(Ctx, false, Culprit);
   EvalResult Result;
   if (EvaluateAsLValue(Result, Ctx) && !Result.HasSideEffects)
 return true;
  diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
  index f0185d0..69b0d0d 100644
  --- a/clang/lib/AST/ExprConstant.cpp
  +++ b/clang/lib/AST/ExprConstant.cpp
  @@ -8381,6 +8381,9 @@ bool LValueExprEvaluator::VisitCallExpr(const CallExpr 
*E) {
  
   bool LValueExprEvaluator::VisitMaterializeTemporaryExpr(
   const MaterializeTemporaryExpr *E) {
  +  if (Info.EvalMode == EvalInfo::EM_ConstantFold)
  +return false;
  +
 // Walk through the expression to find the materialized temporary itself.
 SmallVector CommaLHSs;
 SmallVector Adjustments;
  @@ -8388,8 +8391,8 @@ bool LValueExprEvaluator::VisitMaterializeTemporaryExpr(
 E->getSubExpr()->skipRValueSubobjectAdjustments(CommaLHSs, 
Adjustments);
  
 // If we passed any comma operators, evaluate their LHSs.
  -  for (unsigned I = 0, N = CommaLHSs.size(); I != N; ++I)
  -if (!EvaluateIgnoredValue(Info, CommaLHSs[I]))
  +  for (const Expr *E : CommaLHSs)
  +if (!EvaluateIgnoredValue(Info, E))
 return false;
  
 // A materialized temporary with static storage duration can appear within 
the
  @@ -15472,7 +15475,7 @@ bool Expr::EvaluateAsInitializer(APValue , 
const ASTContext ,
 EStatus.Diag = 
  
 EvalInfo Info(Ctx, EStatus,
  -(IsConstantInitialization && Ctx.getLangOpts().CPlusPlus11)
  +(IsConstantInitialization && Ctx.getLangOpts().CPlusPlus)
   ? EvalInfo::EM_ConstantExpression
   : EvalInfo::EM_ConstantFold);
 Info.setEvaluatingDecl(VD, Value);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: flang/runtime/CMakeLists.txt:251
 
-  INSTALL_WITH_TOOLCHAIN
-)
+if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)
+  add_flang_library(FortranRuntime STATIC

pscoro wrote:
> efriedma wrote:
> > pscoro wrote:
> > > efriedma wrote:
> > > > This "if" doesn't make sense to me.  If we're not building flang-rt, we 
> > > > shouldn't be here, so I don't see why you need an "if" in the first 
> > > > place.
> > > `add_subdirectory(runtime)` is a line that still exists in 
> > > `flang/CMakeLists.txt`. This exists because `Fortran_main` is still being 
> > > built at the same time as the compiler, and to do so, the runtime 
> > > subdirectory still needs to be added to flang (`flang/CMakeLists.txt` -> 
> > > `add_subdirectory(runtime)` -> `flang/runtime/CMakeLists.txt` -> 
> > > `add_subdirectory(FortranMain)`. The solution I had was to just add a 
> > > check around the `FortranRuntime` library production so that it only 
> > > happens for flang-rt.
> > > 
> > > If you have a better solution let me know. Overall, I'm not sure if 
> > > Fortran_main is currently being handled in the best way (ie, its still 
> > > being built at the same time as the compiler, which doesn't seem ideal), 
> > > but am not sure what course of action to take with it since it doesn't 
> > > really belong in flang-rt either (see documentation for details)
> > Fortran_main should be "part of" flang-rt in the sense that building 
> > flang-rt builds it.  Most of the same reasons we want to build flang-rt.a 
> > as a runtime apply.
> > 
> > Since the output needs to be separate, building flang-rt should produce two 
> > libraries: flang-rt.a and FortranMain.a.
> I agree with this idea and have been trying to make it work but in the 
> process I discovered that my "fix" above (`if (DEFINED LLVM_ENABLE_RUNTIMES 
> AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)`) is worse than I thought.
> 
> By building the llvm target with flang-rt as an enabled runtime, we are 
> essentially saying: build the flang compiler, and then invoke cmake again to 
> build the runtimes project (externally), which builds flang-rt.
> 
> The problem is that check-all depends on check-flang which depends on the 
> runtime. The if guard above was not actually doing what I thought it was, and 
> the reason why configuring didnt fail with "flang-rt does not exist" is 
> because the if guard was still evaluating to true. Basically, the guard 
> ensured that FortranRuntime was only being built if flang-rt is built, but 
> the add_library call was still being reached *during the configuration of the 
> flang compiler*.
> 
> I am having trouble dealing with this dependency since runtimes get built 
> externally (and after the compiler is built, which is when the check-all 
> custom target gets built). I will have to investigate more closely how other 
> runtimes handle this. My initial thought was perhaps the runtime testing 
> should be decoupled from check-flang/check-all however I don't know if that's 
> a good idea, and I also think that only helps with flang-rt, but if 
> Fortran_main is required to build any executable I imagine that it should be 
> needed for check-flang?
> 
> This is just an update of whats holding me back right now. If you have any 
> tips please let me know. Thanks for bringing this to my attention.
For comparison, check-clang doesn't require any runtime libraries at all. All 
the checks only check the generated LLVM IR/driver commands/etc.  Any tests 
that require runtime execution go into either the regression tests for the 
runtimes themselves, or into lllvm-test-suite.

Probably check-flang should do something similar. It probably makes sense to 
add a check-flang-rt target or something like that.  (Not sure which existing 
flang tests are impacted.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGVTables.cpp:836
+fnPtr =
+llvm::ConstantExpr::getAddrSpaceCast(fnPtr, CGM.GlobalsInt8PtrTy);
+  return builder.add(fnPtr);

If I follow correctly, the fnPtr is guaranteed to be in the global 
address-space, but GetAddrOfFunction returns a generic pointer.  So the vtable 
entries are in the global address-space for efficiency? Seems reasonable.

There isn't really much point to explicitly checking `FnAS != GVAS` before the 
call to getAddrSpaceCast; getAddrSpaceCast does the same check internally.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153092/new/

https://reviews.llvm.org/D153092

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:8360
+  // Do not constant fold an R-value.
+  if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue())
+return false;

efriedma wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > efriedma wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > efriedma wrote:
> > > > > > > > Checking isLValue() doesn't make sense; consider:
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > struct R { mutable long x; };
> > > > > > > > struct Z { const R , y; };
> > > > > > > > Z z = { R{1}, z.x.x=10 };
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > Maybe also want to check for EM_IgnoreSideEffects?  Not sure 
> > > > > > > > what cases, if any, that would affect.
> > > > > > > > 
> > > > > > > > We should probably check `E->getStorageDuration() == 
> > > > > > > > SD_Static`.  The cases where it's a local temporary don't hit 
> > > > > > > > the getOrCreateValue() codepath, so the evaluated value should 
> > > > > > > > be handled correctly.
> > > > > > > > 
> > > > > > > > Checking EvalMode feels a little weird, but I guess it should 
> > > > > > > > do the right thing in the cases I can think of?  I'd like a 
> > > > > > > > second opinion on this.
> > > > > > > Changing this condition to:
> > > > > > > ```
> > > > > > > if (E->getStorageDuration() == SD_Static &&   
> > > > > > > 
> > > > > > > Info.EvalMode == EvalInfo::EM_ConstantFold && 
> > > > > > > 
> > > > > > > E->isXValue())
> > > > > > > 
> > > > > > >   return false;
> > > > > > > ```
> > > > > > > allows all tests in tree to pass, but messes up the test case you 
> > > > > > > posted above. I'm trying to sus out what else might be different 
> > > > > > > about that test case...we should return `false` for that, but I'm 
> > > > > > > not sure what's different about that case.
> > > > > > > 
> > > > > > > In particular, I was playing with 
> > > > > > > `E->isUsableInConstantExpressions` and 
> > > > > > > `E->getLifetimeExtendedTemporaryDecl()`, but getting your case to 
> > > > > > > work, I end up regressing 
> > > > > > > clang/test/SemaCXX/attr-require-constant-initialization.cpp...argh!!
> > > > > > Shouldn't that just be the following?
> > > > > > 
> > > > > > ```
> > > > > > if (E->getStorageDuration() == SD_Static && 
> > > > > >   
> > > > > > Info.EvalMode == EvalInfo::EM_ConstantFold) 
> > > > > >
> > > > > >   return false;
> > > > > > ```
> > > > > > 
> > > > > > A materialized temporary is always going to be either an LValue or 
> > > > > > an XValue, and the difference between the two isn't relevant here.
> > > > > I wish it were that simple. Checking those two alone will produce 
> > > > > failures in the following tests:
> > > > > 
> > > > > Failed Tests (2):
> > > > >   Clang :: CodeGenCXX/mangle-ms.cpp
> > > > >   Clang :: SemaCXX/attr-require-constant-initialization.cpp
> > > > > 
> > > > > error: 'error' diagnostics seen but not expected: 
> > > > >   File 
> > > > > /android0/llvm-project/clang/test/SemaCXX/attr-require-constant-initialization.cpp
> > > > >  Line 92: variable does not have a constant initializer
> > > > > 
> > > > > as an example of one failure, which is basically:
> > > > > 
> > > > > ```
> > > > > void foo(void) {
> > > > >   __attribute__((require_constant_initialization)) static const int 
> > > > > _init = 42;
> > > > > }
> > > > > ```
> > > > > specifically, `-std=c++03` is the only language version that fails.
> > > > > 
> > > > Oh, perhaps it should simply be:
> > > > 
> > > > ```
> > > > if (Info.EvalMode == EvalInfo::EM_ConstantFold && E->isXValue())
> > > >   return false;
> > > > ```
> > > > 
> > > > let me add your test case for that, too.
> > > It looks like that case is using Expr::isConstantInitializer, which uses 
> > > EM_ConstantFold, which then blows up.  No combination of the checks 
> > > you're trying will let you distinguish between that case and the case 
> > > you're trying to bail out; in both cases, the EvalMode is EvalMode, it's 
> > > an lvalue, and the storage duration is static.
> > > 
> > > Maybe the code in question shouldn't be using isConstantInitializer at 
> > > all.
> > > 
> > > Checking for a reference type doesn't solve anything; it just makes the 
> > > issues more complicated to reproduce.
> > d572f82e490b alludes to `Expr::isConstantInitializer` being error 
> > prone...back in 2011...
> > 
> > > Maybe the code in question shouldn't be using isConstantInitializer at 
> > > all.
> > 
> > Which code? My patch doesn't introduce explicit calls to that method.
> > Which code?
> 
> Sema::CheckCompleteVariableDeclaration
Maybe we can add a special case for 

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:8360
+  // Do not constant fold an R-value.
+  if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue())
+return false;

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > nickdesaulniers wrote:
> > > > > > efriedma wrote:
> > > > > > > Checking isLValue() doesn't make sense; consider:
> > > > > > > 
> > > > > > > ```
> > > > > > > struct R { mutable long x; };
> > > > > > > struct Z { const R , y; };
> > > > > > > Z z = { R{1}, z.x.x=10 };
> > > > > > > ```
> > > > > > > 
> > > > > > > Maybe also want to check for EM_IgnoreSideEffects?  Not sure what 
> > > > > > > cases, if any, that would affect.
> > > > > > > 
> > > > > > > We should probably check `E->getStorageDuration() == SD_Static`.  
> > > > > > > The cases where it's a local temporary don't hit the 
> > > > > > > getOrCreateValue() codepath, so the evaluated value should be 
> > > > > > > handled correctly.
> > > > > > > 
> > > > > > > Checking EvalMode feels a little weird, but I guess it should do 
> > > > > > > the right thing in the cases I can think of?  I'd like a second 
> > > > > > > opinion on this.
> > > > > > Changing this condition to:
> > > > > > ```
> > > > > > if (E->getStorageDuration() == SD_Static && 
> > > > > >   
> > > > > > Info.EvalMode == EvalInfo::EM_ConstantFold &&   
> > > > > >   
> > > > > > E->isXValue())  
> > > > > >   
> > > > > >   return false;
> > > > > > ```
> > > > > > allows all tests in tree to pass, but messes up the test case you 
> > > > > > posted above. I'm trying to sus out what else might be different 
> > > > > > about that test case...we should return `false` for that, but I'm 
> > > > > > not sure what's different about that case.
> > > > > > 
> > > > > > In particular, I was playing with 
> > > > > > `E->isUsableInConstantExpressions` and 
> > > > > > `E->getLifetimeExtendedTemporaryDecl()`, but getting your case to 
> > > > > > work, I end up regressing 
> > > > > > clang/test/SemaCXX/attr-require-constant-initialization.cpp...argh!!
> > > > > Shouldn't that just be the following?
> > > > > 
> > > > > ```
> > > > > if (E->getStorageDuration() == SD_Static &&   
> > > > > 
> > > > > Info.EvalMode == EvalInfo::EM_ConstantFold)   
> > > > >  
> > > > >   return false;
> > > > > ```
> > > > > 
> > > > > A materialized temporary is always going to be either an LValue or an 
> > > > > XValue, and the difference between the two isn't relevant here.
> > > > I wish it were that simple. Checking those two alone will produce 
> > > > failures in the following tests:
> > > > 
> > > > Failed Tests (2):
> > > >   Clang :: CodeGenCXX/mangle-ms.cpp
> > > >   Clang :: SemaCXX/attr-require-constant-initialization.cpp
> > > > 
> > > > error: 'error' diagnostics seen but not expected: 
> > > >   File 
> > > > /android0/llvm-project/clang/test/SemaCXX/attr-require-constant-initialization.cpp
> > > >  Line 92: variable does not have a constant initializer
> > > > 
> > > > as an example of one failure, which is basically:
> > > > 
> > > > ```
> > > > void foo(void) {
> > > >   __attribute__((require_constant_initialization)) static const int 
> > > > _init = 42;
> > > > }
> > > > ```
> > > > specifically, `-std=c++03` is the only language version that fails.
> > > > 
> > > Oh, perhaps it should simply be:
> > > 
> > > ```
> > > if (Info.EvalMode == EvalInfo::EM_ConstantFold && E->isXValue())
> > >   return false;
> > > ```
> > > 
> > > let me add your test case for that, too.
> > It looks like that case is using Expr::isConstantInitializer, which uses 
> > EM_ConstantFold, which then blows up.  No combination of the checks you're 
> > trying will let you distinguish between that case and the case you're 
> > trying to bail out; in both cases, the EvalMode is EvalMode, it's an 
> > lvalue, and the storage duration is static.
> > 
> > Maybe the code in question shouldn't be using isConstantInitializer at all.
> > 
> > Checking for a reference type doesn't solve anything; it just makes the 
> > issues more complicated to reproduce.
> d572f82e490b alludes to `Expr::isConstantInitializer` being error 
> prone...back in 2011...
> 
> > Maybe the code in question shouldn't be using isConstantInitializer at all.
> 
> Which code? My patch doesn't introduce explicit calls to that method.
> Which code?

Sema::CheckCompleteVariableDeclaration


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:8360
+  // Do not constant fold an R-value.
+  if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue())
+return false;

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > Checking isLValue() doesn't make sense; consider:
> > > > > 
> > > > > ```
> > > > > struct R { mutable long x; };
> > > > > struct Z { const R , y; };
> > > > > Z z = { R{1}, z.x.x=10 };
> > > > > ```
> > > > > 
> > > > > Maybe also want to check for EM_IgnoreSideEffects?  Not sure what 
> > > > > cases, if any, that would affect.
> > > > > 
> > > > > We should probably check `E->getStorageDuration() == SD_Static`.  The 
> > > > > cases where it's a local temporary don't hit the getOrCreateValue() 
> > > > > codepath, so the evaluated value should be handled correctly.
> > > > > 
> > > > > Checking EvalMode feels a little weird, but I guess it should do the 
> > > > > right thing in the cases I can think of?  I'd like a second opinion 
> > > > > on this.
> > > > Changing this condition to:
> > > > ```
> > > > if (E->getStorageDuration() == SD_Static && 
> > > >   
> > > > Info.EvalMode == EvalInfo::EM_ConstantFold &&   
> > > >   
> > > > E->isXValue())  
> > > >   
> > > >   return false;
> > > > ```
> > > > allows all tests in tree to pass, but messes up the test case you 
> > > > posted above. I'm trying to sus out what else might be different about 
> > > > that test case...we should return `false` for that, but I'm not sure 
> > > > what's different about that case.
> > > > 
> > > > In particular, I was playing with `E->isUsableInConstantExpressions` 
> > > > and `E->getLifetimeExtendedTemporaryDecl()`, but getting your case to 
> > > > work, I end up regressing 
> > > > clang/test/SemaCXX/attr-require-constant-initialization.cpp...argh!!
> > > Shouldn't that just be the following?
> > > 
> > > ```
> > > if (E->getStorageDuration() == SD_Static &&   
> > > 
> > > Info.EvalMode == EvalInfo::EM_ConstantFold)   
> > >  
> > >   return false;
> > > ```
> > > 
> > > A materialized temporary is always going to be either an LValue or an 
> > > XValue, and the difference between the two isn't relevant here.
> > I wish it were that simple. Checking those two alone will produce failures 
> > in the following tests:
> > 
> > Failed Tests (2):
> >   Clang :: CodeGenCXX/mangle-ms.cpp
> >   Clang :: SemaCXX/attr-require-constant-initialization.cpp
> > 
> > error: 'error' diagnostics seen but not expected: 
> >   File 
> > /android0/llvm-project/clang/test/SemaCXX/attr-require-constant-initialization.cpp
> >  Line 92: variable does not have a constant initializer
> > 
> > as an example of one failure, which is basically:
> > 
> > ```
> > void foo(void) {
> >   __attribute__((require_constant_initialization)) static const int 
> > _init = 42;
> > }
> > ```
> > specifically, `-std=c++03` is the only language version that fails.
> > 
> Oh, perhaps it should simply be:
> 
> ```
> if (Info.EvalMode == EvalInfo::EM_ConstantFold && E->isXValue())
>   return false;
> ```
> 
> let me add your test case for that, too.
It looks like that case is using Expr::isConstantInitializer, which uses 
EM_ConstantFold, which then blows up.  No combination of the checks you're 
trying will let you distinguish between that case and the case you're trying to 
bail out; in both cases, the EvalMode is EvalMode, it's an lvalue, and the 
storage duration is static.

Maybe the code in question shouldn't be using isConstantInitializer at all.

Checking for a reference type doesn't solve anything; it just makes the issues 
more complicated to reproduce.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:8360
+  // Do not constant fold an R-value.
+  if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue())
+return false;

nickdesaulniers wrote:
> efriedma wrote:
> > Checking isLValue() doesn't make sense; consider:
> > 
> > ```
> > struct R { mutable long x; };
> > struct Z { const R , y; };
> > Z z = { R{1}, z.x.x=10 };
> > ```
> > 
> > Maybe also want to check for EM_IgnoreSideEffects?  Not sure what cases, if 
> > any, that would affect.
> > 
> > We should probably check `E->getStorageDuration() == SD_Static`.  The cases 
> > where it's a local temporary don't hit the getOrCreateValue() codepath, so 
> > the evaluated value should be handled correctly.
> > 
> > Checking EvalMode feels a little weird, but I guess it should do the right 
> > thing in the cases I can think of?  I'd like a second opinion on this.
> Changing this condition to:
> ```
> if (E->getStorageDuration() == SD_Static &&   
> Info.EvalMode == EvalInfo::EM_ConstantFold && 
> E->isXValue())
>   return false;
> ```
> allows all tests in tree to pass, but messes up the test case you posted 
> above. I'm trying to sus out what else might be different about that test 
> case...we should return `false` for that, but I'm not sure what's different 
> about that case.
> 
> In particular, I was playing with `E->isUsableInConstantExpressions` and 
> `E->getLifetimeExtendedTemporaryDecl()`, but getting your case to work, I end 
> up regressing 
> clang/test/SemaCXX/attr-require-constant-initialization.cpp...argh!!
Shouldn't that just be the following?

```
if (E->getStorageDuration() == SD_Static &&   
Info.EvalMode == EvalInfo::EM_ConstantFold) 
   
  return false;
```

A materialized temporary is always going to be either an LValue or an XValue, 
and the difference between the two isn't relevant here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > rjmccall wrote:
> > > > nickdesaulniers wrote:
> > > > > rjmccall wrote:
> > > > > > I think it would be good to leave a comment here like this:
> > > > > > 
> > > > > > > We know the possible destinations of an asm goto, but we don't 
> > > > > > > have the
> > > > > > > ability to add code along those control flow edges, so we have to 
> > > > > > > diagnose
> > > > > > > jumps both in and out of scopes, just like we do with an indirect 
> > > > > > > goto.
> > > > > Depending on your definition of "we" (clang vs. llvm), llvm does have 
> > > > > the ability to add code along those edges.
> > > > > 
> > > > > See llvm/lib/CodeGen/CallBrPrepare.cpp.  I'll clarify in your comment 
> > > > > that "clang does not split critical edges during code generation of 
> > > > > llvm ... "
> > > > Okay, so, I don't really know much about this feature.  I was thinking 
> > > > that the branch might go directly into some other assembly block, which 
> > > > would not be splittable.  If the branch just goes to an arbitrary basic 
> > > > block in IR, then it would be straightforward for IRGen to just resolve 
> > > > the destination blocks for `asm goto` labels to some new block that 
> > > > does a normal `goto` to that label.  If we did that, we wouldn't need 
> > > > extra restrictions here at all and could just check this like any other 
> > > > direct branch.
> > > > 
> > > > We don't need to do that work right away, but the comment should 
> > > > probably reflect the full state of affairs — "but clang's IR generation 
> > > > does not currently know how to add code along these control flow edges, 
> > > > so we have to diagnose jumps both in and out of scopes, like we do with 
> > > > indirect goto.  If we ever add that ability to IRGen, this code could 
> > > > check these jumps just like ordinary `goto`s."
> > > > Okay, so, I don't really know much about this feature.
> > > 
> > > "Run this block of asm, then continue to either the next statement or one 
> > > of the explicit labels in the label list."
> > > 
> > > ---
> > > 
> > > That comment still doesn't seem quite right to me.
> > > 
> > > `asm goto` is more like a direct `goto` or a switch in the sense that the 
> > > cases or possible destination are known at compile time; that's not like 
> > > indirect goto where you're jumping to literally anywhere.
> > > 
> > > We need to check the scopes like we would for direct `goto`, because we 
> > > don't want to bypass non-trivial destructors.
> > > 
> > > ---
> > > Interestingly, it looks like some of the cases 
> > > inclang/test/Sema/asm-goto.cpp, `g++` permits, if you use the 
> > > `-fpermissive` flag.  Clang doesn't error that it doesn't recognize that 
> > > flag, but it doesn't seem to do anything in clang, FWICT playing with it 
> > > in godbolt.
> > > 
> > > ---
> > > 
> > > That said, I would have thought
> > > ```
> > > void test4cleanup(int*);
> > > // No errors expected.
> > > void test4(void) {
> > > l0:;
> > > int x __attribute__((cleanup(test4cleanup)));
> > > asm goto("# %l0"l0);
> > > }
> > > ```
> > > To work with this change, but we still produce:
> > > ```
> > > x.c:6:5: error: cannot jump from this asm goto statement to one of its 
> > > possible targets
> > > 6 | asm goto("# %l0"l0);
> > >   | ^
> > > x.c:4:1: note: possible target of asm goto statement
> > > 4 | l0:;
> > >   | ^
> > > x.c:5:9: note: jump exits scope of variable with __attribute__((cleanup))
> > > 5 | int x __attribute__((cleanup(test4cleanup)));
> > >   | ^
> > > ```
> > > Aren't those in the same scope though? I would have expected that to work 
> > > just as if we had a direct `goto l0` rather than the `asm goto`.
> > (There's some history here that the original implementation of asm goto 
> > treated it semantically more like an indirect goto, including the use of 
> > address-of-labels; for a variety of reasons, we changed it so it's more 
> > like a switch statement.)
> > 
> > Suppose we have:
> > 
> > ```
> > void test4cleanup(int*);
> > void test4(void) {
> > asm goto("# %l0"l0);
> > l0:;
> > int x __attribute__((cleanup(test4cleanup)));
> > asm goto("# %l0"l0);
> > }
> > ```
> > 
> > To make this work correctly, the first goto needs to branch directly to the 
> > destination, but the second needs to branch to a call to test4cleanup().  
> > It's probably not that hard to implement: instead of branching directly to 
> > the destination bb, each edge out of the callbr needs to branch to a newly 
> > created block, and that block needs to EmitBranchThroughCleanup() to the 
> > final destination.  (We create such blocks anyway to handle output values, 
> > but 

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: flang/runtime/CMakeLists.txt:251
 
-  INSTALL_WITH_TOOLCHAIN
-)
+if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)
+  add_flang_library(FortranRuntime STATIC

pscoro wrote:
> efriedma wrote:
> > This "if" doesn't make sense to me.  If we're not building flang-rt, we 
> > shouldn't be here, so I don't see why you need an "if" in the first place.
> `add_subdirectory(runtime)` is a line that still exists in 
> `flang/CMakeLists.txt`. This exists because `Fortran_main` is still being 
> built at the same time as the compiler, and to do so, the runtime 
> subdirectory still needs to be added to flang (`flang/CMakeLists.txt` -> 
> `add_subdirectory(runtime)` -> `flang/runtime/CMakeLists.txt` -> 
> `add_subdirectory(FortranMain)`. The solution I had was to just add a check 
> around the `FortranRuntime` library production so that it only happens for 
> flang-rt.
> 
> If you have a better solution let me know. Overall, I'm not sure if 
> Fortran_main is currently being handled in the best way (ie, its still being 
> built at the same time as the compiler, which doesn't seem ideal), but am not 
> sure what course of action to take with it since it doesn't really belong in 
> flang-rt either (see documentation for details)
Fortran_main should be "part of" flang-rt in the sense that building flang-rt 
builds it.  Most of the same reasons we want to build flang-rt.a as a runtime 
apply.

Since the output needs to be separate, building flang-rt should produce two 
libraries: flang-rt.a and FortranMain.a.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

nickdesaulniers wrote:
> rjmccall wrote:
> > nickdesaulniers wrote:
> > > rjmccall wrote:
> > > > I think it would be good to leave a comment here like this:
> > > > 
> > > > > We know the possible destinations of an asm goto, but we don't have 
> > > > > the
> > > > > ability to add code along those control flow edges, so we have to 
> > > > > diagnose
> > > > > jumps both in and out of scopes, just like we do with an indirect 
> > > > > goto.
> > > Depending on your definition of "we" (clang vs. llvm), llvm does have the 
> > > ability to add code along those edges.
> > > 
> > > See llvm/lib/CodeGen/CallBrPrepare.cpp.  I'll clarify in your comment 
> > > that "clang does not split critical edges during code generation of llvm 
> > > ... "
> > Okay, so, I don't really know much about this feature.  I was thinking that 
> > the branch might go directly into some other assembly block, which would 
> > not be splittable.  If the branch just goes to an arbitrary basic block in 
> > IR, then it would be straightforward for IRGen to just resolve the 
> > destination blocks for `asm goto` labels to some new block that does a 
> > normal `goto` to that label.  If we did that, we wouldn't need extra 
> > restrictions here at all and could just check this like any other direct 
> > branch.
> > 
> > We don't need to do that work right away, but the comment should probably 
> > reflect the full state of affairs — "but clang's IR generation does not 
> > currently know how to add code along these control flow edges, so we have 
> > to diagnose jumps both in and out of scopes, like we do with indirect goto. 
> >  If we ever add that ability to IRGen, this code could check these jumps 
> > just like ordinary `goto`s."
> > Okay, so, I don't really know much about this feature.
> 
> "Run this block of asm, then continue to either the next statement or one of 
> the explicit labels in the label list."
> 
> ---
> 
> That comment still doesn't seem quite right to me.
> 
> `asm goto` is more like a direct `goto` or a switch in the sense that the 
> cases or possible destination are known at compile time; that's not like 
> indirect goto where you're jumping to literally anywhere.
> 
> We need to check the scopes like we would for direct `goto`, because we don't 
> want to bypass non-trivial destructors.
> 
> ---
> Interestingly, it looks like some of the cases 
> inclang/test/Sema/asm-goto.cpp, `g++` permits, if you use the `-fpermissive` 
> flag.  Clang doesn't error that it doesn't recognize that flag, but it 
> doesn't seem to do anything in clang, FWICT playing with it in godbolt.
> 
> ---
> 
> That said, I would have thought
> ```
> void test4cleanup(int*);
> // No errors expected.
> void test4(void) {
> l0:;
> int x __attribute__((cleanup(test4cleanup)));
> asm goto("# %l0"l0);
> }
> ```
> To work with this change, but we still produce:
> ```
> x.c:6:5: error: cannot jump from this asm goto statement to one of its 
> possible targets
> 6 | asm goto("# %l0"l0);
>   | ^
> x.c:4:1: note: possible target of asm goto statement
> 4 | l0:;
>   | ^
> x.c:5:9: note: jump exits scope of variable with __attribute__((cleanup))
> 5 | int x __attribute__((cleanup(test4cleanup)));
>   | ^
> ```
> Aren't those in the same scope though? I would have expected that to work 
> just as if we had a direct `goto l0` rather than the `asm goto`.
(There's some history here that the original implementation of asm goto treated 
it semantically more like an indirect goto, including the use of 
address-of-labels; for a variety of reasons, we changed it so it's more like a 
switch statement.)

Suppose we have:

```
void test4cleanup(int*);
void test4(void) {
asm goto("# %l0"l0);
l0:;
int x __attribute__((cleanup(test4cleanup)));
asm goto("# %l0"l0);
}
```

To make this work correctly, the first goto needs to branch directly to the 
destination, but the second needs to branch to a call to test4cleanup().  It's 
probably not that hard to implement: instead of branching directly to the 
destination bb, each edge out of the callbr needs to branch to a newly created 
block, and that block needs to EmitBranchThroughCleanup() to the final 
destination.  (We create such blocks anyway to handle output values, but the 
newly created blocks branch directly to the destination BasicBlock instead of 
using EmitBranchThroughCleanup().)

But until we implement that, we need the error message so we don't miscompile.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: flang/lib/Decimal/CMakeLists.txt:54
+
+if (DEFINED LLVM_ENABLE_PROJECTS AND "flang" IN_LIST LLVM_ENABLE_PROJECTS)
+  add_flang_library(FortranDecimal

I think it would make sense to use explicit CMake variables for the "if' 
statements (i.e. make flang/CMakeLists.txt define BUILDING_FLANG, and make 
flang-rt/CMakeLists define BUILDING_FLANGRT) .



Comment at: flang/runtime/CMakeLists.txt:251
 
-  INSTALL_WITH_TOOLCHAIN
-)
+if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)
+  add_flang_library(FortranRuntime STATIC

This "if" doesn't make sense to me.  If we're not building flang-rt, we 
shouldn't be here, so I don't see why you need an "if" in the first place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155175: [Clang] Fix consteval propagation for aggregates and defaulted constructors

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Test changes look fine.  (I'm not really comfortable reviewing the Sema logic.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155175/new/

https://reviews.llvm.org/D155175

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155175: [Clang] Fix consteval propagation for aggregates and defaulted constructors

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> You have something specific in mind?

I was thinking something along the lines of the original testcase in 63742, 
which successfully compiled in 16, started producing an assertion on main, and 
successfully compiles with this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155175/new/

https://reviews.llvm.org/D155175

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I'm not personally involved with any workflows that care about autoupgrade, so 
I'm not really invested in ensuring it's stable.  If everyone agrees the impact 
is small enough that we're willing to just break autoupgrade to the extent it's 
relevant, I'll withdraw my objection.

> As for the longer term solution to this problem, instead of permitting mixed 
> data layouts of data layout customization, IMO LLVM structs should explicitly 
> encode field offsets. LLVM would still have APIs to assist frontends with 
> producing semi-C-compatible struct layouts, in so much as we do today.

I agree with the general sentiment that we want less dependence on alignment 
specified in the datalayout.  Not sure about the exact design of that for 
structs... if IR moves in the direction people are proposing, the notion of a 
"struct" with a memory layout will likely go away altogether.  (If we remove 
struct types form global variables and GEPs, there's very little left that 
actually cares about the layout of structs in memory.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86310/new/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D86310#4498721 , @hvdijk wrote:

> In D86310#4498575 , @efriedma wrote:
>
>> https://reviews.llvm.org/D86310#2231136 has an example where IR generated by 
>> clang breaks.
>
> clang bases it on the data layout, so when the change here is applied, clang 
> already generates correct IR for that example without further changes (using 
> `%struct.Y = type <{ i64, %struct.X }>`). Unless you were using that as an 
> example of when using old clang to generate LLVM IR, and new LLVM to produce 
> machine code, would break?

I only meant to dispute the assertion that ABI compatibility with old IR is 
only a problem for non-clang frontends.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86310/new/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155175: [Clang] Fix consteval propagation for aggregates and defaulted constructors

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Do we need CodeGen testcases here for full coverage?  The testcases from the 
issue passed with -fsyntax-only.



With this patch, the following cases produce errors that don't really make 
sense to me:

  consteval int f(int x) {
  return x;
  }
  struct SS {
  int y;
  int x = f(this->y);
  constexpr SS(int yy) : y(yy) {}
  };
  SS s = {1};



  :6:13: error: call to consteval function 'f' is not a constant 
expression
  6 | int x = f(this->y);
| ^
  :7:15: note: in the default initializer of 'x'
  7 | constexpr SS(int yy) : y(yy) {}
|   ^
  :6:5: note: declared here
  6 | int x = f(this->y);
| ^
  :6:15: note: use of 'this' pointer is only allowed within the 
evaluation of a call to a 'constexpr' member function
  6 | int x = f(this->y);
|   ^
  1 error generated.



  consteval int f(int x) {
  return x;
  }
  struct SS {
  int y;
  int x = f(this->y);
  consteval SS(int yy) : y(yy) {}
  };
  SS s = {1};

  :6:13: error: cannot take address of consteval function 'f' outside of 
an immediate invocation
  6 | int x = f(this->y);
| ^
  :1:15: note: declared here
  1 | consteval int f(int x) {
|   ^
  1 error generated.

Somehow the following is accepted, though:

  consteval int f(int x) {
  return x;
  }
  struct SS {
  int y;
  int x = f(this->y);
  template constexpr SS(T yy) : y(yy) {}
  };
  SS s = {1};


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155175/new/

https://reviews.llvm.org/D155175

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> The only problem that approach 2 solves is to ensure that a non-clang 
> frontend using i128

https://reviews.llvm.org/D86310#2231136 has an example where IR generated by 
clang breaks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86310/new/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> @efriedma would you consider the changes suggested by @hvdijk sufficient 
> under any circumstances or would you still insist on fully compatible 
> AutoUpgrade, given the above discussion?

If the requirement is "we can mix old and new IR", we have to do it correctly, 
to the extent old versions of clang do it correctly.

If we're willing to refuse to compile old IR and/or refuse to LTO together old 
and new IR, there are other possible solutions.  I'm not sure what workflows 
depend on having working autoupgrade.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86310/new/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits