[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Mostly LGTM. Have several more comments.




Comment at: flang/lib/Lower/Bridge.cpp:279
+// are compiled separately.
+if (hasMainProgram) {
+  createGlobalOutsideOfFunctionLowering([&]() {

Nit



Comment at: flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp:19
+const std::vector &envDefaults) {
+  std::string envDefaultListPtrName =
+  fir::NameUniquer::doEnvironmentDefaultList().str();

```
  if (builder.getNamedGlobal(envDefaultListPtrName))
return;
```
I don't think this and doEnvironmentDefaultList are necessary.



Comment at: flang/test/Driver/emit-mlir.f90:16
 ! CHECK-NEXT: }
+! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : 
!fir.ref, 
!fir.ref> {
+! CHECK-NEXT:  %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

Is it possible not to generated this global variable if `fconvert=` is not 
specified?


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

https://reviews.llvm.org/D130513

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: flang/runtime/environment.cpp:77
+#else
+  envp = environ;
+#endif

What is `environ` used for? Should we try to keep as less extern variable as 
possible in runtime for security.



Comment at: flang/test/Driver/emit-mlir.f90:16
 ! CHECK-NEXT: }
+! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : 
!fir.ref, 
!fir.ref> {
+! CHECK-NEXT:  %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

jpenix-quic wrote:
> peixin wrote:
> > Is it possible not to generated this global variable if `fconvert=` is not 
> > specified?
> I'm not entirely sure--the issue I was running into was how to handle this in 
> Fortran_main.c in a way which worked for all of GCC/Clang/Visual Studio (and 
> maybe others?). I was originally thinking of doing this by using a weak 
> definition of _QQEnvironmentDefaults set to nullptr so fconvert, etc. could 
> override this definition without explicitly generating the fallback case. For 
> GCC/clang, I think I could use __attribute__((weak)), but I wasn't sure how 
> to handle this if someone tried to build with Visual Studio (or maybe another 
> toolchain). I saw a few workarounds (ex: 
> https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) but I shied 
> away from this since it seems to be an undocumented feature (and presumably 
> only helps with Visual Studio). 
> 
> Do you know of a better or more general way I could do this? (Or, is there 
> non-weak symbol approach that might be better that I'm missing?)
How about generate one runtime function with the argument of 
`EnvironmentDefaultList`? This will avoid this and using one extern variable?

If users use one variable with bind C name `_QQEnvironmentDefaults` in fortran 
or one variable with name `_QQEnvironmentDefaults` in C, it is risky. Would 
using the runtime function and static variable with the type 
`EnvironmentDefaultList` in runtime be safer?


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

https://reviews.llvm.org/D130513

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

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In summary:

> If you want to use the updated name, flang, set FLANG_USE_LEGACY_NAME to ON 
> when configuring LLVM Flang.

OFF?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D117835: [OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createSections.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Added @shraiysh and @NimishMishra . They may be more familiar to this code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117835

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Currently, I cannot test the option `-fconvert=big-endian` with open statement 
with convert argument using the following code since lowering is not supported. 
I am not sure if it can be tested in runtime.

  $ cat fconvert_option_openfile.f90 
  module fconvert_option_openfile
  
  contains
subroutine open_for_read(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old')
end
subroutine open_for_read_BE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old', 
convert="BIG_ENDIAN")
end
subroutine open_for_read_LE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old', 
convert="LITTLE_ENDIAN")
end
subroutine open_for_read_native(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old', 
convert="NATIVE")
end
  
subroutine open_for_write(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new')
end
subroutine open_for_write_BE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new', 
convert="BIG_ENDIAN")
end
subroutine open_for_write_LE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new', 
convert="LITTLE_ENDIAN")
end
subroutine open_for_write_native(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new', 
convert="NATIVE")
end
  end
  $ cat fconvert_option_readfile.f90 
  module fconvert_option_readfile
  
  contains
subroutine readfile(fid, del_flag, expect, n, t)
  integer :: n, t
  integer :: fid, del_flag
  real :: expect(n)
  real :: buf(n)
  
  read (fid) buf
  if (del_flag .eq. 0) then
close (fid)
  else
close (fid, status='delete')
  end if
  
  do i = 1, n
if (buf(i) .ne. expect(i)) stop (i+t)
  enddo
end
  end
  $ cat fconvert_option_main_1.f90 
  program fconvert_option_main_1
use fconvert_option_openfile
use fconvert_option_readfile
  
integer, parameter :: n = 4
integer :: del_flag = 0 ! 1 for deleting data file after read
real :: arr(n) = [1.0, 2.0, 3.0, 4.0]
character(:), allocatable :: filename
integer :: arglen
  
call get_command_argument(1, length = arglen)
allocate(character(arglen) :: filename)
call get_command_argument(1, value = filename)
  
call open_for_read(10, filename)
call readfile(10, del_flag, arr, n, 0)
  
call open_for_read_LE(11, filename)
call readfile(11, del_flag, arr, n, 4)
  
call open_for_read_native(12, filename)
call readfile(12, del_flag, arr, n, 8)
  
print *, 'PASS'
  end

BTW, can you continue working on the lowering of the convert argument of open 
statement?




Comment at: clang/include/clang/Driver/Options.td:4897
 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, 
Group, Alias;
+def fconvert_EQ : Joined<["-"], "fconvert=">, Group,
+  HelpText<"Set endian conversion of data for unformatted files">;

awarzynski wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > Why do you move it here? Maybe it is not implemented now, clang may need 
> > > this option eventually. @MaskRay 
> > I was using the fixed line length options as a reference for how to handle 
> > this--based on the discussion in the review here 
> > (https://reviews.llvm.org/D95460) about forwarding options to gfortran, I 
> > was thinking that it would also be safe to handle fconvert similarly, but 
> > I'm not 100% sure and definitely might be misunderstanding something!
> GFortran support has been effectively broken since LLVM 12 (see e.g. this [[ 
> https://github.com/llvm/llvm-project/commit/6a75496836ea14bcfd2f4b59d35a1cad4ac58cee
>  | change ]]). We would do ourselves a favor if we just removed it altogether 
> (I'm not aware of anyone requiring  it). 
> 
> And if Clang ever needs this option, we can always update this definition 
> accordingly. No need to optimize for hypothetical scenarios that may or may 
> not happen :) To me, this change makes perfect sense.
OK. This sounds reasonable to me.



Comment at: flang/lib/Lower/Bridge.cpp:2757
 
+if (funit.isMainProgram()) {
+  auto conversion = bridge.getConversion();

I think this should be moved into `void 
lowerFunc(Fortran::lower::pft::FunctionLikeUnit &funit)`.



Comment at: fla

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: flang/lib/Lower/Bridge.cpp:203
 //them.
 for (Fortran::lower::pft::Program::Units &u : pft.getUnits()) {
   std::visit(Fortran::common::visitors{

Doing this can avoid add one variable to the bridge.


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

https://reviews.llvm.org/D130513

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

This needs rebase.




Comment at: flang/lib/Lower/Bridge.cpp:203
 //them.
 for (Fortran::lower::pft::Program::Units &u : pft.getUnits()) {
   std::visit(Fortran::common::visitors{

jpenix-quic wrote:
> peixin wrote:
> > Doing this can avoid add one variable to the bridge.
> Done! (Although, I added the `isMainProgram()` check/update to the lambda 
> below as it is a member function of `FunctionLikeUnit` specifically--please 
> let me know if this looks ok!)
Yes.


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

https://reviews.llvm.org/D130513

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

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

A few comments. Mostly nits.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3767
+  /*HasMonotonicModifier=*/false, /*HasNonmonotonicModifier=*/false,
+  /*HasOrdedClause=*/false);
   return;





Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:87
+  BaseAuto = 6,
+  BaseRuntime = 5,
+  BaseTrapezoidal = 7,

  BaseRuntime = 5,
  BaseAuto = 6,



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:135
+  UnorderedGuidedSimd = BaseGuidedSimd | ModifierUnordered,   // (46)
+  UnorderedRuntimeSimd = BaseRuntimeSimd | ModifierUnordered, // (47)
+

Why not using the following to be consistent with the name in kmp.h?
StaticBalancedChunked 
GuidedSimd
RuntimeSimd



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2114
+  DL, CLI, AllocaIP, /*NeedsBarrier=*/true, getSchedKind(SchedType),
+  ChunkVal, /*Simd*/ false,
+  (SchedType & omp::OMPScheduleType::ModifierMonotonic) ==

Nit



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2254
+  DL, CLI, AllocaIP, /*NeedsBarrier=*/true, OMP_SCHEDULE_Static, ChunkVal,
+  /*HasSimdModifier*/ false, /*HasMonotonicModifier*/ false,
+  /*HasNonmonotonicModifier*/ false,





Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2255
+  /*HasSimdModifier*/ false, /*HasMonotonicModifier*/ false,
+  /*HasNonmonotonicModifier*/ false,
+  /*HasOrderedClause*/ true);





Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2256
+  /*HasNonmonotonicModifier*/ false,
+  /*HasOrderedClause*/ true);
 





Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:809
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 
%{{.*}}, i32 1073741894, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 
%{{.*}},i32 70, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u

Is this one tab?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123403

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

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

[Peixin Qiao via Phabricator via cfe-commits]

peixin accepted this revision.
peixin added inline comments.
This revision is now accepted and ready to land.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:135
+  UnorderedGuidedSimd = BaseGuidedSimd | ModifierUnordered,   // (46)
+  UnorderedRuntimeSimd = BaseRuntimeSimd | ModifierUnordered, // (47)
+

Meinersbur wrote:
> peixin wrote:
> > Why not using the following to be consistent with the name in kmp.h?
> > StaticBalancedChunked 
> > GuidedSimd
> > RuntimeSimd
> As mentioned in the summary. to avoid confusion by not using the original 
> name. `StaticBalancedChunked` could mean either the algorithm to use (now 
> `BaseStaticBalancedChunked`, as in `omp_sched_t`/`enum kmp_sched`), or that 
> algorithm with the unordered flag set (now `UnorderedStaticBalancedChunked 
> `). I would the former because that's how the enum is structured.
> 
> The name in `kmp.h` for it is actually `kmp_sch_static_balanced_chunked`. 
> `sch` for "unordered"?
OK. Agree.

> The name in kmp.h for it is actually kmp_sch_static_balanced_chunked. sch for 
> "unordered"?

Yes, I think so.  With simd modifier, it cannot be ordered in semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123403

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Except for three nits. LGTM.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505
+  /// valid in the condition block (i.e., defined in the preheader) and is
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount);

Nit: integer -> 64-bit integer?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1676
+  Value *SrcLoc = getOrCreateIdent(getOrCreateSrcLocStr(DL));
+  Value *ThreadNum = getOrCreateThreadID(SrcLoc);
+  Constant *SchedulingType = ConstantInt::get(

peixin wrote:
> Can you move "Value *ThreadNum = getOrCreateThreadID(SrcLoc);" after 
> "Builder.CreateStore(One, PStride);" in order that the 
> "kmpc_global_thread_num" call is right before the "kmpc_static_init" call to 
> keep consistence with others?
This comment is not addressed.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1765
+  switch (SchedKind) {
+  case llvm::omp::ScheduleKind ::OMP_SCHEDULE_Default:
+assert(!ChunkSize && "No chunk size with default schedule (which for clang 
"

peixin wrote:
> Please remove the space between "ScheduleKind" and "OMP_SCHEDULE_Default"? 
> Also for the following switch cases.
The extra space is not removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4897
 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, 
Group, Alias;
+def fconvert_EQ : Joined<["-"], "fconvert=">, Group,
+  HelpText<"Set endian conversion of data for unformatted files">;

Why do you move it here? Maybe it is not implemented now, clang may need this 
option eventually. @MaskRay 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

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

[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

@kiranchandramohan Thanks for the review. Removed the OpenMP 1.0 target from 
the description.

The simd clause does need more changes and I think it also depend simd 
directive in the OpenMPIRBuilder. According to the CHECK5 in the test case 
`ordered_codegen.cpp`, it does not create `kmp_ordered` runtime functions call 
with the simd clause.

I have not investigated too much about the standalone ordered construct. To be 
honest, I am not sure if supporting simd and depend clauses in CreateOrdered is 
better or not. My current plan is to look into the Flang MLIR Op Def and LLVM 
IR to understand how the IRBuilder is used. After finishing the end-to-end 
implementation of ordered construct without simd and depend clauses, I will 
come back to try to implement the IRBuilder of ordered directive with simd and 
depend clauses. At that time, Arnamoy should also have time to implement the 
IRBuilder for simd directive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107430

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

[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder

[Peixin Qiao via Phabricator via cfe-commits]

peixin updated this revision to Diff 364389.
peixin added a comment.

@Meinersbur Thanks for the review.

Removed the typo fixes and gave the alloca register one name.

Also found some typos of invalid case style for variable 'cur' in 
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp and will fix them when pushing 
this patch to main branch.


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

https://reviews.llvm.org/D107430

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/ordered_codegen.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -2113,6 +2113,77 @@
   EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
 }
 
+TEST_F(OpenMPIRBuilderTest, OrderedDirective) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  AllocaInst *PrivAI =
+  Builder.CreateAlloca(F->arg_begin()->getType(), nullptr, "priv.inst");
+
+  BasicBlock *EntryBB = nullptr;
+
+  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+   BasicBlock &FiniBB) {
+EntryBB = FiniBB.getUniquePredecessor();
+
+llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+EXPECT_EQ(EntryBB, CodeGenIPBB);
+
+Builder.restoreIP(CodeGenIP);
+Builder.CreateStore(F->arg_begin(), PrivAI);
+Value *PrivLoad =
+Builder.CreateLoad(PrivAI->getAllocatedType(), PrivAI, "local.use");
+Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+  };
+
+  auto FiniCB = [&](InsertPointTy IP) {
+BasicBlock *IPBB = IP.getBlock();
+EXPECT_NE(IPBB->end(), IP.getPoint());
+  };
+
+  Builder.restoreIP(OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB));
+
+  Value *EntryBBTI = EntryBB->getTerminator();
+  EXPECT_EQ(EntryBBTI, nullptr);
+
+  CallInst *OrderedEntryCI = nullptr;
+  for (auto &EI : *EntryBB) {
+Instruction *Cur = &EI;
+if (isa(Cur)) {
+  OrderedEntryCI = cast(Cur);
+  if (OrderedEntryCI->getCalledFunction()->getName() == "__kmpc_ordered")
+break;
+  OrderedEntryCI = nullptr;
+}
+  }
+  EXPECT_NE(OrderedEntryCI, nullptr);
+  EXPECT_EQ(OrderedEntryCI->getNumArgOperands(), 2U);
+  EXPECT_EQ(OrderedEntryCI->getCalledFunction()->getName(), "__kmpc_ordered");
+  EXPECT_TRUE(isa(OrderedEntryCI->getArgOperand(0)));
+
+  CallInst *OrderedEndCI = nullptr;
+  for (auto &FI : *EntryBB) {
+Instruction *Cur = &FI;
+if (isa(Cur)) {
+  OrderedEndCI = cast(Cur);
+  if (OrderedEndCI->getCalledFunction()->getName() == "__kmpc_end_ordered")
+break;
+  OrderedEndCI = nullptr;
+}
+  }
+  EXPECT_NE(OrderedEndCI, nullptr);
+  EXPECT_EQ(OrderedEndCI->getNumArgOperands(), 2U);
+  EXPECT_TRUE(isa(OrderedEndCI->getArgOperand(0)));
+  EXPECT_EQ(OrderedEndCI->getArgOperand(1), OrderedEntryCI->getArgOperand(1));
+}
+
 TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -1960,6 +1960,30 @@
   /*Conditional*/ false, /*hasFinalize*/ true);
 }
 
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::createOrdered(const LocationDescription &Loc,
+   BodyGenCallbackTy BodyGenCB,
+   FinalizeCallbackTy FiniCB) {
+  if (!updateToLocation(Loc))
+return Loc.IP;
+
+  Directive OMPD = Directive::OMPD_ordered;
+  Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+  Value *Ident = getOrCreateIdent(SrcLocStr);
+  Value *ThreadId = getOrCreateThreadID(Ident);
+  Value *Args[] = {Ident, ThreadId};
+
+  Function *EntryRTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_ordered);
+  Instruction *EntryCall = Builder.CreateCall(EntryRTLFn, Args);
+
+  Function *ExitRTLFn =
+  getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_end_ordered);
+  Instruction *ExitCall = Builder.CreateCall(ExitRTLFn, Args);
+
+  return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
+  /*Conditional*/ false, /*hasFinalize*/ true);
+}
+
 OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::EmitOMPInlinedRegion(
 Directive OMPD, Instruction *EntryCall, Instruction *ExitCall,
 BodyGenCallbackT

[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder

[Peixin Qiao via Phabricator via cfe-commits]

peixin updated this revision to Diff 365170.
peixin added a comment.

Revise the implementation by not generating "kmp_ordered" runtime functions 
when -fopenmp-enable-irbuilder is not enabled, which is consistent with that 
not using OpenMPIRBuilder.


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

https://reviews.llvm.org/D107430

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/ordered_codegen.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -2115,6 +2115,78 @@
   EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
 }
 
+TEST_F(OpenMPIRBuilderTest, OrderedDirective) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+  AllocaInst *PrivAI =
+  Builder.CreateAlloca(F->arg_begin()->getType(), nullptr, "priv.inst");
+
+  BasicBlock *EntryBB = nullptr;
+
+  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+   BasicBlock &FiniBB) {
+EntryBB = FiniBB.getUniquePredecessor();
+
+llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+EXPECT_EQ(EntryBB, CodeGenIPBB);
+
+Builder.restoreIP(CodeGenIP);
+Builder.CreateStore(F->arg_begin(), PrivAI);
+Value *PrivLoad =
+Builder.CreateLoad(PrivAI->getAllocatedType(), PrivAI, "local.use");
+Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+  };
+
+  auto FiniCB = [&](InsertPointTy IP) {
+BasicBlock *IPBB = IP.getBlock();
+EXPECT_NE(IPBB->end(), IP.getPoint());
+  };
+
+  Builder.restoreIP(
+  OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB, false));
+
+  Value *EntryBBTI = EntryBB->getTerminator();
+  EXPECT_EQ(EntryBBTI, nullptr);
+
+  CallInst *OrderedEntryCI = nullptr;
+  for (auto &EI : *EntryBB) {
+Instruction *Cur = &EI;
+if (isa(Cur)) {
+  OrderedEntryCI = cast(Cur);
+  if (OrderedEntryCI->getCalledFunction()->getName() == "__kmpc_ordered")
+break;
+  OrderedEntryCI = nullptr;
+}
+  }
+  EXPECT_NE(OrderedEntryCI, nullptr);
+  EXPECT_EQ(OrderedEntryCI->getNumArgOperands(), 2U);
+  EXPECT_EQ(OrderedEntryCI->getCalledFunction()->getName(), "__kmpc_ordered");
+  EXPECT_TRUE(isa(OrderedEntryCI->getArgOperand(0)));
+
+  CallInst *OrderedEndCI = nullptr;
+  for (auto &FI : *EntryBB) {
+Instruction *Cur = &FI;
+if (isa(Cur)) {
+  OrderedEndCI = cast(Cur);
+  if (OrderedEndCI->getCalledFunction()->getName() == "__kmpc_end_ordered")
+break;
+  OrderedEndCI = nullptr;
+}
+  }
+  EXPECT_NE(OrderedEndCI, nullptr);
+  EXPECT_EQ(OrderedEndCI->getNumArgOperands(), 2U);
+  EXPECT_TRUE(isa(OrderedEndCI->getArgOperand(0)));
+  EXPECT_EQ(OrderedEndCI->getArgOperand(1), OrderedEntryCI->getArgOperand(1));
+}
+
 TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2133,6 +2133,35 @@
   /*Conditional*/ false, /*hasFinalize*/ true);
 }
 
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::createOrdered(const LocationDescription &Loc,
+   BodyGenCallbackTy BodyGenCB,
+   FinalizeCallbackTy FiniCB, bool IsThreads) {
+  if (!updateToLocation(Loc))
+return Loc.IP;
+
+  Directive OMPD = Directive::OMPD_ordered;
+  Instruction *EntryCall = nullptr;
+  Instruction *ExitCall = nullptr;
+
+  if (IsThreads) {
+Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+Value *Ident = getOrCreateIdent(SrcLocStr);
+Value *ThreadId = getOrCreateThreadID(Ident);
+Value *Args[] = {Ident, ThreadId};
+
+Function *EntryRTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_ordered);
+EntryCall = Builder.CreateCall(EntryRTLFn, Args);
+
+Function *ExitRTLFn =
+getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_end_ordered);
+ExitCall = Builder.CreateCall(ExitRTLFn, Args);
+  }
+
+  return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
+  /*Conditional*/ false, /*hasFinalize*/ true);
+}
+
 OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::EmitOMPInlinedRegion(
 Directive OMPD, Instruction *EntryCall, Instruction 

[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

@Meinersbur  Thanks for your review and suggestions.

> While manually editing `ordered_codegen.cpp` gives nicer results, re-running 
> `update_cc_test_checks.py` would be less work. Your manual changes would be 
> also be dropped the next time somebody runs update_cc_test_checks.py and 
> commits.

Thanks for the notice. I followed the other OMPIRBuilder PRs in order to make 
review work easily. It should do not matter my manual changes are dropped the 
next time.

> The code seems derived from @fghanim single/master/etc implementation, would 
> be nice if they could have a look.

Yes. He is in the reviewers list.

> The non-OMPBuilder code emits an outlined `__captured_stmt`, but not with 
> OMPBuilder enabled. I assume this is due to the missing `finatlize` call.

Sorry about the misguide. It is not due to missing `finalize` call. My last 
version of patch only implements the code for `ordered threads`. So I use it 
for `ordered simd` test. The non-OMPIRBuilder code emits the outlined function 
call for `ordered simd`, while emits the inlined statements for `ordered 
threads`. Now the non-OMPIRBuilder code and OMPIRBuilder code generate the 
similar IRs.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:5323-5325
+Range = AssertSuccess(Actions.BuildBinOp(
+nullptr, {}, BO_Add, Range,
+Actions.ActOnIntegerConstant(SourceLocation(), 1).get()));

Meinersbur wrote:
> Haven't really understood why this is necessary, but this new version LGTM.
This is the problem of doing operation ++(Expr A - Expr B), which should be 
replaced with (Expr A - Expr B + "1"). To understand why it is not supportedin 
clang sema, you may need to look at the function stack calls, which I listed as 
follows:

```
SemaOpenMP.cpp: BuildUnaryOp(…Expr…)->
SemaExpr.cpp: 
BuildUnaryOp()->CreateBuiltinUnaryOp()->CheckIncrementDecrementOperand() 
->CheckForModifiableLvalue() {
  Expr::isModifiableLvalueResult IsLV = E->isModifiableLvalue(S.Context, &Loc);
  case Expr::MLV_ClassTemporary:
  DiagID = diag::err_typecheck_expression_not_modifiable_lvalue;
}
```
The root reason is that the temporary expression (Expr A - Expr B) is not 
modifiable LValue. I revised the commit message. Please take a look at it and 
check if it is ok for you.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2154
+  Builder.restoreIP(
+  OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB, false));
+

Meinersbur wrote:
> Did you intend to pass IsThreads=true. Currently it is failing because no 
> `__kmpc_ordered` is emitted.
Yes. Thanks for pointing out the problem.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2156-2157
+
+  Value *EntryBBTI = EntryBB->getTerminator();
+  EXPECT_EQ(EntryBBTI, nullptr);
+

Meinersbur wrote:
> Consider emitting a terminator, call `finalize()` and `verifyModule`.
Why do you want me to emit the terminator? If it is because you think the 
outlined captured function is not generated due to finalize call, there is no 
need. Discussed above. Sorry about the misguide.


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

https://reviews.llvm.org/D107430

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

@Meinersbur Please rebase on main. The function "getPreheader()" was moved into 
OMPIRBuilder.h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D114413#3159095 , @Meinersbur 
wrote:

> In D114413#3154936 , @peixin wrote:
>
>> @Meinersbur Please rebase on main. The function "getPreheader()" was moved 
>> into OMPIRBuilder.h.
>
> I rebased, but I am not sure what you are referring to. `getPreheader()` 
> always was in `OMPIRBuilder.h`

`getPreheader()` was in `OMPIRBuilder.cpp` before you rebase in your last 
update here. That's why I let you rebase since I failed to git apply your last 
patch in main branch. It's not important now. Please forget about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D114413#3166979 , @Meinersbur 
wrote:

> In D114413#3160044 , @peixin wrote:
>
>> In D114413#3159095 , @Meinersbur 
>> wrote:
>>
>>> In D114413#3154936 , @peixin 
>>> wrote:
>>>
 
>>
>> `getPreheader()` was in `OMPIRBuilder.cpp` before you rebase in your last 
>> update here. That's why I let you rebase since I failed to git apply your 
>> last patch in main branch. It's not important now. Please forget about that.
>
> D114368  (which this patch depends on) 
> moves `getPreheder()` to the `.cpp` files (because it has become more than a 
> simple getter)

Thanks a lot. Now I get it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D131808: [clang,flang] Add missing options fsyntax-only in help

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

The CI failed since the test cases in flang driver have new output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131808

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

This needs rebase. I failed to apply this patch due to conflict.


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

https://reviews.llvm.org/D130513

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

  $ cat fconvert_option_main_2.f90 
  !
  ! Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
  ! See https://llvm.org/LICENSE.txt for license information.
  ! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  !
  ! checking for I/O: testing read with -fconvert=big-endian.
  ! The convert argument of open function has prior claim over fconvert option.
  ! Only main program intentionally compiled with -fconvert=big-endian.
  
  program fconvert_option_main_2
use fconvert_option_openfile
use fconvert_option_readfile
  
integer, parameter :: n = 4
integer :: del_flag = 0 ! 1 for deleting data file after read
real :: arr(n) = [1.0, 2.0, 3.0, 4.0]
character(:), allocatable :: filename
integer :: arglen
  
call get_command_argument(1, length = arglen)
allocate(character(arglen) :: filename)
call get_command_argument(1, value = filename)
  
call open_for_read_LE(10, filename)
call readfile(10, del_flag, arr, n, 0)
  
call open_for_read_native(11, filename)
call readfile(11, del_flag, arr, n, 4)
  
print *, 'PASS'
  end
  $ cat fconvert_option_readfile.f90 
  !
  ! Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
  ! See https://llvm.org/LICENSE.txt for license information.
  ! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  !
  ! checking for I/O: readfile subroutine
  
  module fconvert_option_readfile
  
  contains
subroutine readfile(fid, del_flag, expect, n, t)
  integer :: n, t
  integer :: fid, del_flag
  real :: expect(n)
  real :: buf(n)
  
  read (fid) buf
  if (del_flag .eq. 0) then
close (fid)
  else
close (fid, status='delete')
  end if
  
  do i = 1, n
if (buf(i) .ne. expect(i)) stop (i+t)
  enddo
end
  end
  $ cat fconvert_option_openfile.f90 
  !
  ! Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
  ! See https://llvm.org/LICENSE.txt for license information.
  ! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  !
  ! checking for I/O: openfile subroutines
  
  module fconvert_option_openfile
  
  contains
subroutine open_for_read(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old')
end
subroutine open_for_read_BE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old', 
convert="BIG_ENDIAN")
end
subroutine open_for_read_LE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old', 
convert="LITTLE_ENDIAN")
end
subroutine open_for_read_native(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='old', 
convert="NATIVE")
end
  
subroutine open_for_write(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new')
end
subroutine open_for_write_BE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new', 
convert="BIG_ENDIAN")
end
subroutine open_for_write_LE(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new', 
convert="LITTLE_ENDIAN")
end
subroutine open_for_write_native(fid, file_name)
  integer :: fid
  character(:), allocatable :: file_name
  open (fid, file=file_name, form='UNFORMATTED', status='new', 
convert="NATIVE")
end
  end
  $ cat fconvert_option_writefile.f90 
  !
  ! Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
  ! See https://llvm.org/LICENSE.txt for license information.
  ! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  !
  ! checking for I/O: writefile subroutine
  
  module fconvert_option_writefile
  
  contains
subroutine writefile(fid, buf, n)
  integer :: n
  real :: buf(n)
  integer :: fid
  
  write (fid) buf
  close (fid)
end
  end
  $ cat run.sh 
  #!/bin/bash
  
  echo "-- gfortran --"
  gfortran fconvert_option_readfile.f90 -c
  gfortran fconvert_option_openfile.f90 -c
  gfortran fconvert_option_writefile.f90 -c
  gfortran $1.f90 $2 -c
  gfortran $1.o fconvert_option_readfile.o fconvert_option_openfile.o 
fconvert_option_writefile.o
  ./a.out Inputs/fc_opt_data_$3 && rm *.o *.mod a.out
  
  echo && echo "-- flang-new --"
  flang-new fconvert_option_readfile.f90 -c
  flang-new fconvert_option_openfile.f90 -c
  flang-new fconvert_option_writefile.f90 -c
  flang-new $1.f90 $2 -c
  flang-new -flang-expe

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: flang/runtime/environment.cpp:77
+#else
+  envp = environ;
+#endif

jpenix-quic wrote:
> peixin wrote:
> > What is `environ` used for? Should we try to keep as less extern variable 
> > as possible in runtime for security.
> I think `setenv` is only required to make sure that the `environ` pointer 
> points to the correct copy of the environment variables. So, as long as we 
> are storing a copy of the environment pointer in `ExecutionEnvironment` and 
> potentially making changes to the environment via `setenv`, I think we need 
> to base it off the `environ` pointer after `setenv` has been called rather 
> than the `envp` from `main`.
> 
> That said, I don't think the `envp` variable we are storing is being used for 
> anything at the moment (reading environment variables was changed to use 
> `getenv` rather than `envp` in the commit [[ 
> https://github.com/llvm/llvm-project/commit/824bf908194c9267f1f09065ba8e41d7969006ab
>  | here]]). If removing the usage of `environ` is preferable, we could maybe 
> remove the usage of `envp` altogether (in a separate patch)--does this sound 
> like a good idea or would it be better to just leave in the `environ` usage 
> for now?
Thanks for the explanations. The current fix makes sense to me.



Comment at: flang/test/Driver/emit-mlir.f90:16
 ! CHECK-NEXT: }
+! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : 
!fir.ref, 
!fir.ref> {
+! CHECK-NEXT:  %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

jpenix-quic wrote:
> peixin wrote:
> > jpenix-quic wrote:
> > > peixin wrote:
> > > > Is it possible not to generated this global variable if `fconvert=` is 
> > > > not specified?
> > > I'm not entirely sure--the issue I was running into was how to handle 
> > > this in Fortran_main.c in a way which worked for all of GCC/Clang/Visual 
> > > Studio (and maybe others?). I was originally thinking of doing this by 
> > > using a weak definition of _QQEnvironmentDefaults set to nullptr so 
> > > fconvert, etc. could override this definition without explicitly 
> > > generating the fallback case. For GCC/clang, I think I could use 
> > > __attribute__((weak)), but I wasn't sure how to handle this if someone 
> > > tried to build with Visual Studio (or maybe another toolchain). I saw a 
> > > few workarounds (ex: 
> > > https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) but I 
> > > shied away from this since it seems to be an undocumented feature (and 
> > > presumably only helps with Visual Studio). 
> > > 
> > > Do you know of a better or more general way I could do this? (Or, is 
> > > there non-weak symbol approach that might be better that I'm missing?)
> > How about generate one runtime function with the argument of 
> > `EnvironmentDefaultList`? This will avoid this and using one extern 
> > variable?
> > 
> > If users use one variable with bind C name `_QQEnvironmentDefaults` in 
> > fortran or one variable with name `_QQEnvironmentDefaults` in C, it is 
> > risky. Would using the runtime function and static variable with the type 
> > `EnvironmentDefaultList` in runtime be safer?
> Agreed that there are potential risks with the current approach (although, 
> are the `_Q*` names considered reserved?). Unfortunately, I think generating 
> a call to set the environment defaults requires somewhat significant changes 
> to the runtime. The runtime reads environment variables during initialization 
> in `ExecutionEnvironment::Configure` which is ultimately called from the 
> "hardcoded" `Fortran_main.c` and I need to set the defaults before this 
> happens. So, I believe I'd either have to move the initialization to 
> `_QQmain`  or make it so that `main` isn't hardcoded so that I could insert 
> the appropriate runtime function.
> 
> @klausler I think I asked you about this when I was first trying to figure 
> out how to implement the environment defaults and you suggested I try the 
> extern approach--please let me know if you have thoughts/suggestions around 
> this!
This is what @klausler suggested:
```
Instead of adding new custom APIs that let command-line options control 
behavior in a way that is redundant with the runtime environment, I suggest 
that you try a more general runtime library API by which the main program can 
specify a default environment variable setting, or a set of them. Then turn the 
command-line options into the equivalent environment settings and pass them as 
default settings that could be overridden by the actual environment.
```
If I understand correctly, what I am suggesting match his comments. The "main 
program" he means should be fortran main program, not the 
`RTNAME(ProgramStart`. In your initial patch, you add the runtime specified for 
"convert option". I think @klausler suggest you making the runtime argument 
more general used for a set of runtime environment variable settings, not 
restricte

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: flang/test/Driver/emit-mlir.f90:16
 ! CHECK-NEXT: }
+! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : 
!fir.ref, 
!fir.ref> {
+! CHECK-NEXT:  %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

jpenix-quic wrote:
> peixin wrote:
> > jpenix-quic wrote:
> > > peixin wrote:
> > > > jpenix-quic wrote:
> > > > > peixin wrote:
> > > > > > Is it possible not to generated this global variable if `fconvert=` 
> > > > > > is not specified?
> > > > > I'm not entirely sure--the issue I was running into was how to handle 
> > > > > this in Fortran_main.c in a way which worked for all of 
> > > > > GCC/Clang/Visual Studio (and maybe others?). I was originally 
> > > > > thinking of doing this by using a weak definition of 
> > > > > _QQEnvironmentDefaults set to nullptr so fconvert, etc. could 
> > > > > override this definition without explicitly generating the fallback 
> > > > > case. For GCC/clang, I think I could use __attribute__((weak)), but I 
> > > > > wasn't sure how to handle this if someone tried to build with Visual 
> > > > > Studio (or maybe another toolchain). I saw a few workarounds (ex: 
> > > > > https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) but 
> > > > > I shied away from this since it seems to be an undocumented feature 
> > > > > (and presumably only helps with Visual Studio). 
> > > > > 
> > > > > Do you know of a better or more general way I could do this? (Or, is 
> > > > > there non-weak symbol approach that might be better that I'm missing?)
> > > > How about generate one runtime function with the argument of 
> > > > `EnvironmentDefaultList`? This will avoid this and using one extern 
> > > > variable?
> > > > 
> > > > If users use one variable with bind C name `_QQEnvironmentDefaults` in 
> > > > fortran or one variable with name `_QQEnvironmentDefaults` in C, it is 
> > > > risky. Would using the runtime function and static variable with the 
> > > > type `EnvironmentDefaultList` in runtime be safer?
> > > Agreed that there are potential risks with the current approach 
> > > (although, are the `_Q*` names considered reserved?). Unfortunately, I 
> > > think generating a call to set the environment defaults requires somewhat 
> > > significant changes to the runtime. The runtime reads environment 
> > > variables during initialization in `ExecutionEnvironment::Configure` 
> > > which is ultimately called from the "hardcoded" `Fortran_main.c` and I 
> > > need to set the defaults before this happens. So, I believe I'd either 
> > > have to move the initialization to `_QQmain`  or make it so that `main` 
> > > isn't hardcoded so that I could insert the appropriate runtime function.
> > > 
> > > @klausler I think I asked you about this when I was first trying to 
> > > figure out how to implement the environment defaults and you suggested I 
> > > try the extern approach--please let me know if you have 
> > > thoughts/suggestions around this!
> > This is what @klausler suggested:
> > ```
> > Instead of adding new custom APIs that let command-line options control 
> > behavior in a way that is redundant with the runtime environment, I suggest 
> > that you try a more general runtime library API by which the main program 
> > can specify a default environment variable setting, or a set of them. Then 
> > turn the command-line options into the equivalent environment settings and 
> > pass them as default settings that could be overridden by the actual 
> > environment.
> > ```
> > If I understand correctly, what I am suggesting match his comments. The 
> > "main program" he means should be fortran main program, not the 
> > `RTNAME(ProgramStart`. In your initial patch, you add the runtime specified 
> > for "convert option". I think @klausler suggest you making the runtime 
> > argument more general used for a set of runtime environment variable 
> > settings, not restricted to "convert option". And that is what you already 
> > added -- `EnvironmentDefaultList`. So, combining this patch and your 
> > initial patch will be the solution. Hope I understand it correctly.
> The issue I hit with the suggested approach is that in order to use the 
> pre-existing runtime environment variable handling to set the internal state 
> I need to set the environment variable defaults before the environment 
> variables are read by the runtime.
> 
> I might be misunderstanding/missing something, but given that the environment 
> variables are read as part of `RTNAME(ProgramStart)` in `main` and the 
> earliest I can place the call if I am generating it is `_QQmain`, I think 
> that leaves three options: 1. don't hardcode `main` so that I can place the 
> call early enough 2. delay or rerun the code [[ 
> https://github.com/llvm/llvm-project/blob/c619d4f840dcba54751ff8c5aaafce0f173a4ad5/flang/runtime/environment.cpp#L50-L90
>  | here ]] that is responsible for initializing the runtime state so that 

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: flang/test/Driver/emit-mlir.f90:16
 ! CHECK-NEXT: }
+! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : 
!fir.ref, 
!fir.ref> {
+! CHECK-NEXT:  %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

awarzynski wrote:
> peixin wrote:
> > jpenix-quic wrote:
> > > peixin wrote:
> > > > jpenix-quic wrote:
> > > > > peixin wrote:
> > > > > > jpenix-quic wrote:
> > > > > > > peixin wrote:
> > > > > > > > Is it possible not to generated this global variable if 
> > > > > > > > `fconvert=` is not specified?
> > > > > > > I'm not entirely sure--the issue I was running into was how to 
> > > > > > > handle this in Fortran_main.c in a way which worked for all of 
> > > > > > > GCC/Clang/Visual Studio (and maybe others?). I was originally 
> > > > > > > thinking of doing this by using a weak definition of 
> > > > > > > _QQEnvironmentDefaults set to nullptr so fconvert, etc. could 
> > > > > > > override this definition without explicitly generating the 
> > > > > > > fallback case. For GCC/clang, I think I could use 
> > > > > > > __attribute__((weak)), but I wasn't sure how to handle this if 
> > > > > > > someone tried to build with Visual Studio (or maybe another 
> > > > > > > toolchain). I saw a few workarounds (ex: 
> > > > > > > https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) 
> > > > > > > but I shied away from this since it seems to be an undocumented 
> > > > > > > feature (and presumably only helps with Visual Studio). 
> > > > > > > 
> > > > > > > Do you know of a better or more general way I could do this? (Or, 
> > > > > > > is there non-weak symbol approach that might be better that I'm 
> > > > > > > missing?)
> > > > > > How about generate one runtime function with the argument of 
> > > > > > `EnvironmentDefaultList`? This will avoid this and using one extern 
> > > > > > variable?
> > > > > > 
> > > > > > If users use one variable with bind C name `_QQEnvironmentDefaults` 
> > > > > > in fortran or one variable with name `_QQEnvironmentDefaults` in C, 
> > > > > > it is risky. Would using the runtime function and static variable 
> > > > > > with the type `EnvironmentDefaultList` in runtime be safer?
> > > > > Agreed that there are potential risks with the current approach 
> > > > > (although, are the `_Q*` names considered reserved?). Unfortunately, 
> > > > > I think generating a call to set the environment defaults requires 
> > > > > somewhat significant changes to the runtime. The runtime reads 
> > > > > environment variables during initialization in 
> > > > > `ExecutionEnvironment::Configure` which is ultimately called from the 
> > > > > "hardcoded" `Fortran_main.c` and I need to set the defaults before 
> > > > > this happens. So, I believe I'd either have to move the 
> > > > > initialization to `_QQmain`  or make it so that `main` isn't 
> > > > > hardcoded so that I could insert the appropriate runtime function.
> > > > > 
> > > > > @klausler I think I asked you about this when I was first trying to 
> > > > > figure out how to implement the environment defaults and you 
> > > > > suggested I try the extern approach--please let me know if you have 
> > > > > thoughts/suggestions around this!
> > > > This is what @klausler suggested:
> > > > ```
> > > > Instead of adding new custom APIs that let command-line options control 
> > > > behavior in a way that is redundant with the runtime environment, I 
> > > > suggest that you try a more general runtime library API by which the 
> > > > main program can specify a default environment variable setting, or a 
> > > > set of them. Then turn the command-line options into the equivalent 
> > > > environment settings and pass them as default settings that could be 
> > > > overridden by the actual environment.
> > > > ```
> > > > If I understand correctly, what I am suggesting match his comments. The 
> > > > "main program" he means should be fortran main program, not the 
> > > > `RTNAME(ProgramStart`. In your initial patch, you add the runtime 
> > > > specified for "convert option". I think @klausler suggest you making 
> > > > the runtime argument more general used for a set of runtime environment 
> > > > variable settings, not restricted to "convert option". And that is what 
> > > > you already added -- `EnvironmentDefaultList`. So, combining this patch 
> > > > and your initial patch will be the solution. Hope I understand it 
> > > > correctly.
> > > The issue I hit with the suggested approach is that in order to use the 
> > > pre-existing runtime environment variable handling to set the internal 
> > > state I need to set the environment variable defaults before the 
> > > environment variables are read by the runtime.
> > > 
> > > I might be misunderstanding/missing something, but given that the 
> > > environment variables are read as part of `RTNAME(ProgramStart)` in 
> > > `main` and the earliest I can place the call if I a

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

The build still fails.


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

https://reviews.llvm.org/D130513

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D130513#3804659 , @jpenix-quic 
wrote:

>> The build still fails.
>
> I think it might be an infrastructure issue or something like that--I've 
> tried restarting it a few times and each time it just ends with "HTTP 28" as 
> the only message I see. Another review that was created a bit ago 
> (https://reviews.llvm.org/D134329) seems to have the same issue, so I'll try 
> restarting it again in a while.

All the new created reviews fail for the same reason. 
https://reviews.llvm.org/harbormaster. You may need to wait for some time to 
restart it.


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

https://reviews.llvm.org/D130513

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

LGTM. Tested on one little-endian machine with our internal tests (I attached 
them in the issue https://github.com/llvm/llvm-project/issues/55961) for the 
priority of `-fconvert=` option and `CONVERT` argument in open statement and 
all passed. I didn't see the the `GFORTRAN_CONVERT_UNIT` used in HPC workloads 
and don't have tests for it. For now, I would avoid full combination tests for 
it.

@jeanPerier @clementval Can you help double check the lowering part?


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

https://reviews.llvm.org/D130513

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

[PATCH] D134821: [flang][driver] Allow main program to be in an archive

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Please fix the clang-format issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134821

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

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

The CI failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

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

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

OK. This seems not to be related to this patch. 
https://buildkite.com/llvm-project/premerge-checks/builds/98446#0181715d-812b-4d2a-b5d0-5c1283d78b5f
 I also see these failures in another review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

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

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Thanks for this patch. This is one big progress to use `flang-new` for real 
workloads. Mostly looks good to me. I have several minor comments. If supporing 
`-Ofast` requires more changes, I prefer to push forward with the current patch.

summary: fronted -> frontend

Confirmed with the following case:

  program m
integer :: x = 1, y = 2, z = 0
call add(x, y, z)
print *, z
  contains
subroutine add(a, b, c)
  integer :: a, b, c
  integer :: i
  do i = 1, 10
c = c + a + b
  enddo
end
  end

  $ flang-new -fc1 -emit-llvm test.f90 -O3
  $ cat test.ll 
  ; ModuleID = 'FIRModule'
  source_filename = "FIRModule"
  target triple = "aarch64-unknown-linux-gnu"
  
  @_QFEz = internal unnamed_addr global i32 0
  @_QQcl.2E2F746573742E66393000 = linkonce constant [11 x i8] c"./test.f90\00"
  
  define void @_QQmain() local_unnamed_addr !dbg !3 {
%1 = load i32, ptr @_QFEz, align 4, !dbg !7
%2 = add i32 %1, 30, !dbg !7
store i32 %2, ptr @_QFEz, align 4, !dbg !7
%3 = tail call ptr @_FortranAioBeginExternalListOutput(i32 -1, ptr nonnull 
@_QQcl.2E2F746573742E66393000, i32 5), !dbg !12
%4 = tail call i1 @_FortranAioOutputInteger32(ptr %3, i32 %2), !dbg !13
%5 = tail call i32 @_FortranAioEndIoStatement(ptr %3), !dbg !12
ret void, !dbg !14
  }
  ; Function Attrs: argmemonly nofree norecurse nosync nounwind
  define void @_QFPadd(ptr nocapture readonly %0, ptr nocapture readonly %1, 
ptr nocapture %2) local_unnamed_addr #0 !dbg !9 {
 ...




Comment at: clang/include/clang/Driver/Options.td:732
+def O_flag : Flag<["-"], "O">, Flags<[CC1Option,FC1Option]>, Alias, 
AliasArgs<["1"]>;
 def Ofast : Joined<["-"], "Ofast">, Group, Flags<[CC1Option]>;
 def P : Flag<["-"], "P">, Flags<[CC1Option,FlangOption,FC1Option]>, 
Group,

Will enabling Ofast require more changes in the flang frontend? If yes, it is 
OK to support it in another patch later.



Comment at: flang/include/flang/Frontend/CodeGenOptions.def:30
+
+VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
+

I saw `clang/include/clang/Basic/CodeGenOptions.def` has the following:
```
VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
VALUE_CODEGENOPT(OptimizeSize, 2, 0) ///< If -Os (==1) or -Oz (==2) is 
specified.
```

Do `-Os` and `-Oz` need extra processing in flang drivers? If yes, it is OK to 
support it in another patch later.



Comment at: flang/lib/Frontend/CodeGenOptions.cpp:8
+//===--===//
+
+#include "flang/Frontend/CodeGenOptions.h"

Miss the following?
```
//
// Coding style: https://mlir.llvm.org/getting_started/DeveloperGuide/
//
//===--===//
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

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

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Peixin Qiao via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/include/clang/Driver/Options.td:732
+def O_flag : Flag<["-"], "O">, Flags<[CC1Option,FC1Option]>, Alias, 
AliasArgs<["1"]>;
 def Ofast : Joined<["-"], "Ofast">, Group, Flags<[CC1Option]>;
 def P : Flag<["-"], "P">, Flags<[CC1Option,FlangOption,FC1Option]>, 
Group,

awarzynski wrote:
> peixin wrote:
> > Will enabling Ofast require more changes in the flang frontend? If yes, it 
> > is OK to support it in another patch later.
> I would much prefer to have it implemented it in a dedicated patch.
> 
> For every option, one has to consider the semantics in the compiler driver 
> (`flang-new`) vs frontend driver (`flang-new -fc1`) and then, in case of 
> code-gen flags, the difference between middle-end and back-end pass 
> pipelines. So quite a few things :)
> 
> In general, I'm in favor of doing this incrementally, in small patches. This 
> makes reviewing and testing easier and more self-contained. Is that OK?
> In general, I'm in favor of doing this incrementally, in small patches. This 
> makes reviewing and testing easier and more self-contained. Is that OK?

Cannot agree more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

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

[PATCH] D126164: [flang][Driver] Refine _when_ driver diagnostics are formatted

[Peixin Qiao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG430841605d49: [flang][Driver] Refine _when_ driver 
diagnostics are formatted (authored by peixin).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126164

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/Driver/color-diagnostics-forwarding.f90
  flang/test/Driver/color-diagnostics.f90
  flang/test/Driver/driver-help.f90

Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -83,6 +83,7 @@
 ! HELP-FC1-NEXT: -falternative-parameter-statement
 ! HELP-FC1-NEXT: Enable the old style PARAMETER statement
 ! HELP-FC1-NEXT: -fbackslashSpecify that backslash in string introduces an escape character
+! HELP-FC1-NEXT: -fcolor-diagnostics Enable colors in diagnostics
 ! HELP-FC1-NEXT: -fdebug-dump-all   Dump symbols and the parse tree after the semantic checks
 ! HELP-FC1-NEXT: -fdebug-dump-parse-tree-no-sema
 ! HELP-FC1-NEXT:Dump the parse tree (skips the semantic checks)
Index: flang/test/Driver/color-diagnostics.f90
===
--- /dev/null
+++ flang/test/Driver/color-diagnostics.f90
@@ -0,0 +1,23 @@
+! Test the behaviors of -f{no-}color-diagnostics.
+! Windows command prompt doesn't support ANSI escape sequences.
+! REQUIRES: shell
+
+! RUN: not %flang %s -fcolor-diagnostics 2>&1 \
+! RUN: | FileCheck %s --check-prefix=CHECK_CD
+! RUN: not %flang %s -fno-color-diagnostics 2>&1 \
+! RUN: | FileCheck %s --check-prefix=CHECK_NCD
+! RUN: not %flang_fc1 %s -fcolor-diagnostics 2>&1 \
+! RUN: | FileCheck %s --check-prefix=CHECK_CD
+! RUN: not %flang_fc1 %s -fno-color-diagnostics 2>&1 \
+! RUN: | FileCheck %s --check-prefix=UNSUPPORTED_OPTION
+! RUN: not %flang_fc1 %s 2>&1 | FileCheck %s --check-prefix=CHECK_NCD
+
+! CHECK_CD: {{.*}}[0;1;31merror: {{.*}}[0m{{.*}}[1mSemantic errors in {{.*}}color-diagnostics.f90{{.*}}[0m
+
+! CHECK_NCD: Semantic errors in {{.*}}color-diagnostics.f90
+
+! UNSUPPORTED_OPTION: error: unknown argument: '-fno-color-diagnostics'
+
+program m
+  integer :: i = k
+end
Index: flang/test/Driver/color-diagnostics-forwarding.f90
===
--- /dev/null
+++ flang/test/Driver/color-diagnostics-forwarding.f90
@@ -0,0 +1,21 @@
+! Test that flang-new forwards -f{no-}color-diagnostics options to
+! flang-new -fc1 as expected.
+
+! RUN: %flang -fsyntax-only -### %s -o %t 2>&1 -fcolor-diagnostics \
+! RUN:   | FileCheck %s --check-prefix=CHECK-CD
+! CHECK-CD: "-fc1"{{.*}} "-fcolor-diagnostics"
+
+! RUN: %flang -fsyntax-only -### %s -o %t 2>&1 -fno-color-diagnostics \
+! RUN:   | FileCheck %s --check-prefix=CHECK-NCD
+! CHECK-NCD-NOT: "-fc1"{{.*}} "-fcolor-diagnostics"
+
+! Check that the last flag wins.
+! RUN: %flang -fsyntax-only -### %s -o %t 2>&1 \
+! RUN: -fno-color-diagnostics -fcolor-diagnostics \
+! RUN:   | FileCheck %s --check-prefix=CHECK-NCD_CD_S
+! CHECK-NCD_CD_S: "-fc1"{{.*}} "-fcolor-diagnostics"
+
+! RUN: %flang -fsyntax-only -### %s -o %t 2>&1 \
+! RUN: -fcolor-diagnostics -fno-color-diagnostics \
+! RUN:   | FileCheck %s --check-prefix=CHECK-CD_NCD_S
+! CHECK-CD_NCD_S-NOT: "-fc1"{{.*}} "-fcolor-diagnostics"
Index: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
===
--- flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
+++ flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
@@ -158,6 +158,9 @@
 return false;
   }
 
+  // Honor color diagnostics.
+  flang->getDiagnosticOpts().ShowColors = flang->getFrontendOpts().showColors;
+
   // Create and execute the frontend action.
   std::unique_ptr act(createFrontendAction(*flang));
   if (!act)
Index: flang/lib/Frontend/CompilerInvocation.cpp
===
--- flang/lib/Frontend/CompilerInvocation.cpp
+++ flang/lib/Frontend/CompilerInvocation.cpp
@@ -53,10 +53,11 @@
 //===--===//
 // Deserialization (from args)
 //===--===//
-static bool parseShowColorsArgs(
-const llvm::opt::ArgList &args, bool defaultColor) {
-  // Color diagnostics default to auto ("on" if terminal supports) in the driver
-  // but default to off in cc1, needing an explicit OPT_fdiagnostics_color.
+static bool par

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

LGTM




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:133
+  CmdArgs.push_back("-O3");
+  TC.getDriver().Diag(diag::warn_O4_is_O3);
+} else {

Nit: I have committed D126164, and you can rebase and use D directly, which is 
the style in `Clang.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

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

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D125788#3535199 , @sscalpone wrote:

> My proposal is:
>
> If the compiler compiles it, it ought to run.
> If the compiler can't compile it, it ought to clearly say why.
>
> 1. All tests of legal Fortran that compile & link must also execute correctly 
> (which excludes tests that expect to catch a problem at runtime)
> 2. For all tests with unsupported features, the compiler must issues an error 
> message and the message references the source-location of the unsupported 
> feature
>
> My preference is to use the NAG test suite.   It is not freely available.

I tested a lot of test cases (mostly Fortran 95 code) and have several detailed 
questions about the reasonable quality bar.

1. Should some incorrect execution results be changed into one TODO if there is 
no plan to support it soon? One example is derived type array in forall 
(https://github.com/flang-compiler/f18-llvm-project/issues/1598).

2. What if the case fails in lowering with a "fatal internal error", but the 
real reason is the incorrect semantic analysis? Should it either be supported, 
or be turned from the "fatal internal error" into TODO?

3. What about the edge cases? For those with incorrect execution results, turn 
them into TODO? One example is D125632 .

4. What about some dangerous usage? Usually, gfortran can report a lot of 
warnings for the dangerous scenarios such as string length mismatch, external 
procedure type kind mismatch, the test case in D125891 
, etc. Maybe this is not in priority.

5. Are the rules also applied to Fortran 2003, 2008 and 2018 code, or only 
restricted to Fortran 95 code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D134821: [flang][driver] Allow main program to be in an archive

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

> @peixin , wdyt?

@awarzynski Hi Andrzej, sorry for the late reply. I am distracted by several 
internal projects and other things in my life recently (just came back from 
vacation). My boss has not decided to let me continue working on LLVM Flang 
this year, yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134821

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Can you rebase this? I cannot apply this patch on current main branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

When I investigated the edge cases you mentioned in D116292 
. Found one unsupported case as follows

  #include 
  #include 
  using namespace std;
  
  void func(unsigned long long lb, unsigned long long ub, unsigned long long 
step) {
unsigned long long i;
#pragma omp for schedule(static, 1)
for (i = lb; i > ub; i -= step) {
  cout << i << endl;
}
  }
  
  int main() {
unsigned long long lb, ub, step;
lb = ULLONG_MAX;
ub = ULLONG_MAX / 10;
step = ULLONG_MAX / 10;
cout << "lb: " << lb << endl;
cout << "ub: " << ub << endl;
cout << "step: " << step << endl;
  
func(lb, ub, step);
  
cout << endl;
return 0;
  }

  $ clang++ temp.cpp -fopenmp && ./a.out
  lb: 18446744073709551615
  ub: 1844674407370955161
  step: 1844674407370955161
  18446744073709551615
  16602069666338596454
  14757395258967641293
  12912720851596686132
  11068046444225730971
  9223372036854775810
  7378697629483820649
  5534023222112865488
  3689348814741910327
  1844674407370955166
  $ clang++ temp.cpp -fopenmp -fopenmp-enable-irbuilder
  clang-14: 
/home/qpx/compilers/llvm-community/static-chunk-codegen/llvm-project/llvm/lib/IR/Instructions.cpp:506:
 void llvm::CallInst::init(llvm::FunctionType*, llvm::Value*, 
llvm::ArrayRef, 
llvm::ArrayRef >, const llvm::Twine&): 
Assertion `(i >= FTy->getNumParams() || FTy->getParamType(i) == 
Args[i]->getType()) && "Calling a function with a bad signature!"' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:

This is also for `schedule(static)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D118542: [Clang][OpenMPIRBuilder] Fix off-by-one error when dividing by stepsize.

[Peixin Qiao via Phabricator via cfe-commits]

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

Function code LGTM. Please fix the CI fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118542

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Thanks for the fix. The fix of off-by-one issue looks ok to me. Will continue 
reviewing other parts in one week due to the Spring Festival in China.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.
Herald added a project: All.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505
+  /// valid in the condition block (i.e., defined in the preheader) and is
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount);

Meinersbur wrote:
> peixin wrote:
> > Nit: integer -> 64-bit integer?
> not necessarily, we do not require a specific integer size. For instance, 
> `__kmpc_for_static_init_4u` takes a 32-bit integer. It is up to the applyXYZ 
> function to zext/trunc it when necessary.
Got it. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D121101: [clang] Fix OpenMP critical hint parameter check

[Peixin Qiao via Phabricator via cfe-commits]

peixin created this revision.
peixin added a reviewer: ABataev.
peixin added a project: OpenMP.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
peixin requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

The paramemter of hint clause in OpenMP critical hint should be
non-negative. The omp_lock_hint_none is 0 in omp.h.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121101

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/critical_messages.cpp


Index: clang/test/OpenMP/critical_messages.cpp
===
--- clang/test/OpenMP/critical_messages.cpp
+++ clang/test/OpenMP/critical_messages.cpp
@@ -67,10 +67,14 @@
   foo();
   #pragma omp critical (name2) hint(argc) // expected-error {{integral 
constant expression}} expected-note 0+{{constant expression}}
   foo();
-  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' 
clause must be a strictly positive integer value}} expected-error {{constructs 
with the same name must have a 'hint' clause with the same value}} 
expected-note {{'hint' clause with value '4'}}
+  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' 
clause must be a non-negative integer value}} expected-error {{constructs with 
the same name must have a 'hint' clause with the same value}} expected-note 
{{'hint' clause with value '4'}}
   foo();
   #pragma omp critical hint(N) // expected-error {{the name of the construct 
must be specified in presence of 'hint' clause}}
   foo();
+
+  const int omp_lock_hint_none = 0;
+  #pragma omp critical (name3) hint(omp_lock_hint_none)
+  foo();
   return 0;
 }
 
@@ -132,7 +136,7 @@
   foo();
   #pragma omp critical (name) hint(23) // expected-note {{previous 'hint' 
clause with value '23'}}
   foo();
-  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause 
must be a strictly positive integer value}}
+  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause 
must be a non-negative integer value}}
   foo();
   #pragma omp critical hint(1) // expected-error {{the name of the construct 
must be specified in presence of 'hint' clause}}
   foo();
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -21402,7 +21402,8 @@
   // OpenMP [2.13.2, critical construct, Description]
   // ... where hint-expression is an integer constant expression that evaluates
   // to a valid lock hint.
-  ExprResult HintExpr = VerifyPositiveIntegerConstantInClause(Hint, OMPC_hint);
+  ExprResult HintExpr =
+  VerifyPositiveIntegerConstantInClause(Hint, OMPC_hint, false);
   if (HintExpr.isInvalid())
 return nullptr;
   return new (Context)


Index: clang/test/OpenMP/critical_messages.cpp
===
--- clang/test/OpenMP/critical_messages.cpp
+++ clang/test/OpenMP/critical_messages.cpp
@@ -67,10 +67,14 @@
   foo();
   #pragma omp critical (name2) hint(argc) // expected-error {{integral constant expression}} expected-note 0+{{constant expression}}
   foo();
-  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' clause must be a strictly positive integer value}} expected-error {{constructs with the same name must have a 'hint' clause with the same value}} expected-note {{'hint' clause with value '4'}}
+  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' clause must be a non-negative integer value}} expected-error {{constructs with the same name must have a 'hint' clause with the same value}} expected-note {{'hint' clause with value '4'}}
   foo();
   #pragma omp critical hint(N) // expected-error {{the name of the construct must be specified in presence of 'hint' clause}}
   foo();
+
+  const int omp_lock_hint_none = 0;
+  #pragma omp critical (name3) hint(omp_lock_hint_none)
+  foo();
   return 0;
 }
 
@@ -132,7 +136,7 @@
   foo();
   #pragma omp critical (name) hint(23) // expected-note {{previous 'hint' clause with value '23'}}
   foo();
-  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause must be a strictly positive integer value}}
+  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause must be a non-negative integer value}}
   foo();
   #pragma omp critical hint(1) // expected-error {{the name of the construct must be specified in presence of 'hint' clause}}
   foo();
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -21402,7 +21402,8 @@
   // OpenMP [2.13.2, critical construct, Description]
   // ... where hint-expression is an integer constant expression that evaluates
   // to a va

[PATCH] D121101: [clang] Fix OpenMP critical hint parameter check

[Peixin Qiao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4e159e4c7b97: [clang] Fix OpenMP critical hint parameter 
check (authored by peixin).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121101

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/critical_messages.cpp


Index: clang/test/OpenMP/critical_messages.cpp
===
--- clang/test/OpenMP/critical_messages.cpp
+++ clang/test/OpenMP/critical_messages.cpp
@@ -67,10 +67,14 @@
   foo();
   #pragma omp critical (name2) hint(argc) // expected-error {{integral 
constant expression}} expected-note 0+{{constant expression}}
   foo();
-  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' 
clause must be a strictly positive integer value}} expected-error {{constructs 
with the same name must have a 'hint' clause with the same value}} 
expected-note {{'hint' clause with value '4'}}
+  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' 
clause must be a non-negative integer value}} expected-error {{constructs with 
the same name must have a 'hint' clause with the same value}} expected-note 
{{'hint' clause with value '4'}}
   foo();
   #pragma omp critical hint(N) // expected-error {{the name of the construct 
must be specified in presence of 'hint' clause}}
   foo();
+
+  const int omp_lock_hint_none = 0;
+  #pragma omp critical (name3) hint(omp_lock_hint_none)
+  foo();
   return 0;
 }
 
@@ -132,7 +136,7 @@
   foo();
   #pragma omp critical (name) hint(23) // expected-note {{previous 'hint' 
clause with value '23'}}
   foo();
-  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause 
must be a strictly positive integer value}}
+  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause 
must be a non-negative integer value}}
   foo();
   #pragma omp critical hint(1) // expected-error {{the name of the construct 
must be specified in presence of 'hint' clause}}
   foo();
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -21402,7 +21402,8 @@
   // OpenMP [2.13.2, critical construct, Description]
   // ... where hint-expression is an integer constant expression that evaluates
   // to a valid lock hint.
-  ExprResult HintExpr = VerifyPositiveIntegerConstantInClause(Hint, OMPC_hint);
+  ExprResult HintExpr =
+  VerifyPositiveIntegerConstantInClause(Hint, OMPC_hint, false);
   if (HintExpr.isInvalid())
 return nullptr;
   return new (Context)


Index: clang/test/OpenMP/critical_messages.cpp
===
--- clang/test/OpenMP/critical_messages.cpp
+++ clang/test/OpenMP/critical_messages.cpp
@@ -67,10 +67,14 @@
   foo();
   #pragma omp critical (name2) hint(argc) // expected-error {{integral constant expression}} expected-note 0+{{constant expression}}
   foo();
-  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' clause must be a strictly positive integer value}} expected-error {{constructs with the same name must have a 'hint' clause with the same value}} expected-note {{'hint' clause with value '4'}}
+  #pragma omp critical (name) hint(N) // expected-error {{argument to 'hint' clause must be a non-negative integer value}} expected-error {{constructs with the same name must have a 'hint' clause with the same value}} expected-note {{'hint' clause with value '4'}}
   foo();
   #pragma omp critical hint(N) // expected-error {{the name of the construct must be specified in presence of 'hint' clause}}
   foo();
+
+  const int omp_lock_hint_none = 0;
+  #pragma omp critical (name3) hint(omp_lock_hint_none)
+  foo();
   return 0;
 }
 
@@ -132,7 +136,7 @@
   foo();
   #pragma omp critical (name) hint(23) // expected-note {{previous 'hint' clause with value '23'}}
   foo();
-  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause must be a strictly positive integer value}}
+  #pragma omp critical hint(-5) // expected-error {{argument to 'hint' clause must be a non-negative integer value}}
   foo();
   #pragma omp critical hint(1) // expected-error {{the name of the construct must be specified in presence of 'hint' clause}}
   foo();
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -21402,7 +21402,8 @@
   // OpenMP [2.13.2, critical construct, Description]
   // ... where hint-expression is an integer constant expression that evaluates
   // to a valid lock hint.
-  ExprResult HintExpr = VerifyPositiveIntegerConstantInClause(Hint, OMPC_hint);
+  ExprResult HintExpr =
+  VerifyPositiveIntegerConstantInClause(Hint, OMPC_hint, false);
   if (H

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.
Herald added a subscriber: awarzynski.

@Meinersbur Here is the c++ code test. Without the chunk size specified, the 
running result using OMPIRBuilder is correct.

  #include
  
  int main() {
int i, N = 10;
float x = 0.0;
  
#pragma omp for schedule(static, 2)
for(i = 3; i <= N; i++) {
  x = x + i;
}
  
std::cout << "x = " << x << std::endl;
  
return 0;
  }

  $ clang++ chunk-1.cpp -fopenmp -fopenmp-enable-irbuilder && ./a.out
  x = 7
  $ clang++ chunk-1.cpp -fopenmp && ./a.out
  x = 52


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

BTW, please rebase on main. There is one conflict about function 
`getOrCreateSrcLocStr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Can you check the following example by applying this patch on fir-dev?

  program main
integer :: i, N = 10
real :: x = 0
  
!$omp do schedule(static, 2)
do i = 3, N
  x = x + i
end do
!$omp end do
  
print *, x
  end

Test running result:

  $ gfortran test.f90 -fopenmp && ./a.out
 52.000
  $ bbc -fopenmp -emit-fir test.f90
  $ tco test.mlir -o test.ll
  $ clang++ -lFortran_main -lFortranRuntime -lFortranDecimal -lomp -o a.out 
test.ll
  $ ./a.out
   7.

When you change "schedule(static, 2)" into "schedule(static, 1)", the running 
result is 3.0.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1676
+  Value *SrcLoc = getOrCreateIdent(getOrCreateSrcLocStr(DL));
+  Value *ThreadNum = getOrCreateThreadID(SrcLoc);
+  Constant *SchedulingType = ConstantInt::get(

Can you move "Value *ThreadNum = getOrCreateThreadID(SrcLoc);" after 
"Builder.CreateStore(One, PStride);" in order that the "kmpc_global_thread_num" 
call is right before the "kmpc_static_init" call to keep consistence with 
others?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1765
+  switch (SchedKind) {
+  case llvm::omp::ScheduleKind ::OMP_SCHEDULE_Default:
+assert(!ChunkSize && "No chunk size with default schedule (which for clang 
"

Please remove the space between "ScheduleKind" and "OMP_SCHEDULE_Default"? Also 
for the following switch cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114413

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

[PATCH] D111992: [MLIR][OpenMP] Added omp.atomic.read and omp.atomic.write

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5993
   case OMPC_adjust_args:
+  case OMPC_memory_order:
 llvm_unreachable("Clause is not allowed in 'omp atomic'.");

The memory_order clause in clang side is not handled in this patch. Maybe 
leaving the error is better since users will know it is not supported yet in 
clang.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:513
+
+def AtomicReadOp : OpenMP_Op<"atomic.read"> {
+  let arguments = (ins OpenMP_PointerLikeType:$address,

How do you plan to handle synchronization between the threads if there is no 
region in atomic read op? Will there be one implicit `kmpc_flush` function call 
before `!$omp end atomic`? If yes, it is easier to find to location of emiting 
`kmpc_flush` if we have one region here. Please let me know if I am wrong. 



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:1246
+StringRef memOrder = op.memory_order().getValue();
+if (memOrder.equals("acq_rel") || memOrder.equals("release"))
+  return op.emitError(

Please also check OpenMP 5.1 Spec. It seems that `acq_rel` is allowed if read 
clause is specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

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

[PATCH] D111992: [MLIR][OpenMP] Added omp.atomic.read and omp.atomic.write

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5993
   case OMPC_adjust_args:
+  case OMPC_memory_order:
 llvm_unreachable("Clause is not allowed in 'omp atomic'.");

peixin wrote:
> The memory_order clause in clang side is not handled in this patch. Maybe 
> leaving the error is better since users will know it is not supported yet in 
> clang.
> The memory_order clause in clang side is not handled in this patch. Maybe 
> leaving the error is better since users will know it is not supported yet in 
> clang.

Sorry. Please ignore this comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

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

[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

@Meinersbur  Thanks a lot for your review and accepting this patch.
BTW, I finished the implementation of the codegen of ordered construct for LLVM 
Flang and the OpenMP IRBuilder of ordered construct in this patch also works 
well for LLVM Flang. Is it OK to land this patch now or does it need more 
review?


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

https://reviews.llvm.org/D107430

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

[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Here is the PR for codegen of ordered construct for LLVM Flang: 
https://github.com/flang-compiler/f18-llvm-project/pull/1027.
I create the PR on fir-dev branch since the test of lowering parse-tree to MLIR 
for LLVM Flang is still not supported in upstream llvm-project.


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

https://reviews.llvm.org/D107430

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

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

[Peixin Qiao via Phabricator via cfe-commits]

peixin created this revision.
peixin added reviewers: jhuber6, jdoerfert, kiranchandramohan, Meinersbur, 
clementval, kiranktp, Leporacanthicus, bryanpkc, arnamoy10, Chuanfeng.
peixin added projects: LLVM, clang.
Herald added subscribers: guansong, yaxunl.
peixin requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.

For the ordered construct with the simd clause, the outlined region
cannot be inlined. Otherwise, there may be unexpected behavior when
the captured statements are vectorized.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109321

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp


Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -5362,8 +5362,8 @@
   CGF.CapturedStmtInfo = &CapStmtInfo;
   llvm::Function *Fn = CGF.GenerateOpenMPCapturedStmtFunction(*S, Loc);
   Fn->setDoesNotRecurse();
-  if (CGM.getCodeGenOpts().OptimizationLevel != 0)
-Fn->addFnAttr(llvm::Attribute::AlwaysInline);
+  Fn->removeFnAttr(llvm::Attribute::AlwaysInline);
+  Fn->addFnAttr(llvm::Attribute::NoInline);
   return Fn;
 }
 


Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -5362,8 +5362,8 @@
   CGF.CapturedStmtInfo = &CapStmtInfo;
   llvm::Function *Fn = CGF.GenerateOpenMPCapturedStmtFunction(*S, Loc);
   Fn->setDoesNotRecurse();
-  if (CGM.getCodeGenOpts().OptimizationLevel != 0)
-Fn->addFnAttr(llvm::Attribute::AlwaysInline);
+  Fn->removeFnAttr(llvm::Attribute::AlwaysInline);
+  Fn->addFnAttr(llvm::Attribute::NoInline);
   return Fn;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

The following test case fails after 
https://reviews.llvm.org/rGaf000197c4214926bd7d0862d86f89aed5f20da6.

  #include 
  using namespace std;
  
  int main() {
float a[10];
int i, N = 10;
for (i = 0; i < N; i++)
  a[i] = 0;
  
#pragma omp simd
for (i = 0; i < N; i++) {
  #pragma omp ordered simd
  a[i] = a[i-1] + 1.0;
}
  
for (i = 0; i < N; i++)
  cout << a[i] << "  ";
cout << endl;
  }



  $ clang++ -fopenmp simd.cpp && ./a.out
  1  2  3  4  5  6  7  8  9  10
  $ clang++ -fopenmp -O1 simd.cpp && ./a.out
  1  1  1  1  2  1  1  1  2  3

It is fixed by this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109321

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

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

`for (i = 0; i < N; i++)` --> `for (i = 1; i < N; i++)`

  #include 
  using namespace std;
  
  int main() {
float a[10];
int i, N = 10;
for (i = 0; i < N; i++)
  a[i] = 0;
  
#pragma omp simd
for (i = 1; i < N; i++) {
  #pragma omp ordered simd
  a[i] = a[i-1] + 1.0;
}
  
for (i = 0; i < N; i++)
  cout << a[i] << "  ";
cout << endl;
  }



  $ clang++ -fopenmp simd.cpp && ./a.out
  0 1  2  3  4  5  6  7  8  9
  $ clang++ -fopenmp -O1 simd.cpp && ./a.out
  0 1  1  1  1  2  1  1  1  2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109321

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

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D109321#2985284 , @jhuber6 wrote:

> In D109321#2985281 , @lebedev.ri 
> wrote:
>
>> Aha. But i don't think this is the right fix,
>> the fact that the inlining manifests the miscompile is a symptom.
>
> Preventing the outlined region from being inlined would also hurt OpenMP 
> performance considerably.

Please notice that this remove is only inside `emitOutlinedOrderedFunction`, 
which is only used when `ordered simd` directive is there. According to OpenMP 
5.0 Spec, the ordered construct either specifies a structured block in a 
worksharing-loop, simd, or worksharing-loop SIMD region that will be executed 
in the order of the loop iterations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109321

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

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

  $ clang++ -fopenmp simd.cpp -O1 -Xclang -disable-llvm-passes && ./a.out
  0 1  2  3  4  5  6  7  8  9
  $ clang++ -fopenmp simd.cpp -O2 && ./a.out
  0 1  2  3  4  5  6  7  8  9
  $ clang++ -fopenmp simd.cpp -O3 && ./a.out
  0 1  2  3  4  5  6  7  8  9

This bug is not in clang frontend. I will post it in bugzilla.

Another question is why not add `llvm::Attribute::AlwaysInline` when 
`CGM.getCodeGenOpts().OptimizationLevel` is 0? @jhuber6 I think it is correct 
to add the attribute when `CGM.getCodeGenOpts().OptimizationLevel` is 0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109321

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

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D109321#2987783 , @jdoerfert wrote:

> As noted before, this is not the right fix. Not inlining should not cause a 
> semantic difference here, the problem is something else.

@jdoerfert Yes, I agree that this is not the right fix. What I mean is to 
abandon this PR and post this bug in bugzilla to let someone who knows more 
about optimization passes to solve it considering this may not be one frontend 
bug according to the result of the command `clang++ -fopenmp simd.cpp -O1 
-Xclang -disable-llvm-passes`. I have to admit that always inlining the 
outlined function gives correct results since optmization passes vectorize the 
statements if there is no dependence (eg. a[i] = 1.0) and add memcheck if there 
may be dependence (eg. a[i] = b[i] + 1.0). Although this vectorization violate 
the definition of `ordered` construct in OpenMP 5.0 standard, it's safe to do 
it. Anyway, I think it's not one big problem here.

> Just to prevent the IR from changing when optimizations aren't enabled.

@jhuber6  Thanks for the reply. I think not generating outlined function is the 
right way when there is no optimization. Using the code for `ordered threads` 
like the following should be ok. This should not be one big deal since few 
users use `O0` to run their applications. Do you think if we should make this 
change?

  diff --git a/clang/lib/CodeGen/CGStmtOpenMP.cpp 
b/clang/lib/CodeGen/CGStmtOpenMP.cpp
  index 6ede4c3189d4..0ca5c7b71084 100644
  --- a/clang/lib/CodeGen/CGStmtOpenMP.cpp
  +++ b/clang/lib/CodeGen/CGStmtOpenMP.cpp
  @@ -5443,11 +5443,12 @@ void CodeGenFunction::EmitOMPOrderedDirective(const 
OMPOrderedDirective &S) {
 CGM.getOpenMPRuntime().emitDoacrossOrdered(*this, DC);
   return;
 }
  +  unsigned OptLevel = CGM.getCodeGenOpts().OptimizationLevel;
 const auto *C = S.getSingleClause();
  -  auto &&CodeGen = [&S, C, this](CodeGenFunction &CGF,
  - PrePostActionTy &Action) {
  +  auto &&CodeGen = [&S, OptLevel, C, this](CodeGenFunction &CGF,
  +   PrePostActionTy &Action) {
   const CapturedStmt *CS = S.getInnermostCapturedStmt();
  -if (C) {
  +if (C && OptLevel > 0) {
 llvm::SmallVector CapturedVars;
 CGF.GenerateOpenMPCapturedVars(*CS, CapturedVars);
 llvm::Function *OutlinedFn =


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109321

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

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

[Peixin Qiao via Phabricator via cfe-commits]

peixin abandoned this revision.
peixin added a comment.

Abandon this PR since this is not right fix. Continuing discussion will be on 
openmp-dev mail list. This bug should be fixed if `ordered simd` is processed 
correctly in frontend and vectorization pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109321

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