[PATCH] D75661: Remove SequentialType from the type heirarchy.
ftynse added a comment.
LGTM for MLIR part.
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:60
+ } else {
emitError(loc) << "expected sequential LLVM types wrapping a scalar";
return nullptr;
Nit: can we update the error message not to mention "sequential LLVM type"
anymore if sequential type is no longer a thing?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75661/new/
https://reviews.llvm.org/D75661
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78882: [IR] Replace all uses of CallBase::getCalledValue() with getCalledOperand().
ftynse accepted this revision. ftynse added a comment. This revision is now accepted and ready to land. LGTM for MLIR Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78882/new/ https://reviews.llvm.org/D78882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D121076: [MLIR][Python] Add SCFIfOp Python binding
ftynse accepted this revision. ftynse added a comment. Thanks! Let me know if you need help landing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121076/new/ https://reviews.llvm.org/D121076 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91556: [OpenMPIRBuilder} Add capturing of parameters to pass to omp::parallel
ftynse added a comment.
Thanks! I have a couple of comments, but I will defer to @jdoerfert for
approval in any case.
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:676
+
+ /// Capture the above-defined paraneters for the parallel regions.
+ ///
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:678
+ ///
+ /// \param CaptureAllocaInsPoint Insertion point for the alloca-ed struct.
+ /// \param OuterFn The function containing the omp::Parallel.
Nit: it looks like this file uses IP rather than InsPoint for names related to
insertion points
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:760
+ SetVector BlockParents;
+ for (unsigned Counter = 0; Counter < Blocks.size(); Counter++) {
+BasicBlock *ParallelRegionBlock = Blocks[Counter];
Nit: I think
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
applies to `.size()` the same way it applies to `.end()`
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:795
+ // captured values with the extracted ones from the struct.
+ if (CapturedValues.size()) {
+// Create the StructTy
Nit:
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
```
if (CapturedValues.empty())
return;
```
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:796
+ if (CapturedValues.size()) {
+// Create the StructTy
+std::vector StructTypes;
Nit: trailing dot
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:799
+for (unsigned Counter = 0; Counter < CapturedValues.size(); Counter++)
+ StructTypes.push_back(CapturedValues[Counter]->getType());
+
Nit: please `reserve` before pushing back in a loop
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:807-808
+ llvm::IRBuilder<>::InsertPointGuard Guard(Builder);
+ Builder.SetInsertPoint(CaptureAllocaInsPoint.getBlock(),
+ CaptureAllocaInsPoint.getPoint());
+ // Allocate and populate the capture struct.
Nit: `Builder.restoreIP(CaptureAllocaInsPoint)` looks shorter
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:810-815
+ AllocaInst = Builder.CreateAlloca(CaptureStructType, nullptr,
+"CaptureStructAlloca");
+ Value *InsertValue = UndefValue::get(CaptureStructType);
+ for (auto SrcIdx : enumerate(CapturedValues))
+InsertValue = Builder.CreateInsertValue(InsertValue, SrcIdx.value(),
+SrcIdx.index());
I suppose you may want to have `alloca` inserted in a block (function entry)
different from the one where you store into the memory. You need to store just
before calling the fork function (or, at least, so that the store postdominates
all stored values). Looking at the function API, I would have assumed
`CaptureAllocaInsPoint` to be an insertion point at the function entry block
specifically for `alloca`s, where these `insertvalue`s are invalid.
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:826-829
+ for (unsigned Counter = 0; Counter < Blocks.size(); Counter++)
+for (auto I = Blocks[Counter]->begin(), E = Blocks[Counter]->end();
+ I != E; ++I)
+ for (Use &U : I->operands())
Can we rather take each captured value and enumerate its uses, replacing those
within the parallel block set?
Comment at: llvm/test/CodeGen/XCore/threads.ll:84-140
+; This test fails on Windows because the second and third
+; register of the add operations are swapped.
+; Windows test is below.
+; REQUIRES: !windows
define i32* @f_tle() {
; CHECK-LABEL: f_tle:
; CHECK: get r11, id
These look irrelevant to the patch, but seem to fix a breakage upstream. Would
you mind committing this separately?
Comment at: mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir:1
+// RUN: mlir-translate --mlir-to-llvmir %s | FileCheck %s
+
Changes to MLIR are no longer necessary
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91556/new/
https://reviews.llvm.org/D91556
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref
ftynse added a comment. Herald added a subscriber: teijeong. LGTM for the MLIR part Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91410/new/ https://reviews.llvm.org/D91410 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
ftynse updated this revision to Diff 308643.
ftynse added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Fix clang tests. The order of arguments is switched in the internal outlined
function.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92189/new/
https://reviews.llvm.org/D92189
Files:
clang/test/OpenMP/parallel_codegen.cpp
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
@@ -60,6 +60,25 @@
DebugLoc DL;
};
+// Returns the value stored in the given allocation. Returns null if the given
+// value is not a result of an allocation, if no value is stored or if there is
+// more than one store.
+static Value *findStoredValue(Value *AllocaValue) {
+ Instruction *Alloca = dyn_cast(AllocaValue);
+ if (!Alloca)
+return nullptr;
+ StoreInst *Store = nullptr;
+ for (Use &U : Alloca->uses()) {
+if (auto *CandidateStore = dyn_cast(U.getUser())) {
+ EXPECT_EQ(Store, nullptr);
+ Store = CandidateStore;
+}
+ }
+ if (!Store)
+return nullptr;
+ return Store->getValueOperand();
+};
+
TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
OpenMPIRBuilder OMPBuilder(*M);
OMPBuilder.initialize();
@@ -401,7 +420,7 @@
EXPECT_EQ(ForkCI->getArgOperand(1),
ConstantInt::get(Type::getInt32Ty(Ctx), 1U));
EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
- EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
+ EXPECT_EQ(findStoredValue(ForkCI->getArgOperand(3)), F->arg_begin());
}
TEST_F(OpenMPIRBuilderTest, ParallelNested) {
@@ -708,13 +727,15 @@
EXPECT_TRUE(isa(ForkCI->getArgOperand(0)));
EXPECT_EQ(ForkCI->getArgOperand(1),
ConstantInt::get(Type::getInt32Ty(Ctx), 1));
- EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
+ Value *StoredForkArg = findStoredValue(ForkCI->getArgOperand(3));
+ EXPECT_EQ(StoredForkArg, F->arg_begin());
EXPECT_EQ(DirectCI->getCalledFunction(), OutlinedFn);
EXPECT_EQ(DirectCI->getNumArgOperands(), 3U);
EXPECT_TRUE(isa(DirectCI->getArgOperand(0)));
EXPECT_TRUE(isa(DirectCI->getArgOperand(1)));
- EXPECT_EQ(DirectCI->getArgOperand(2), F->arg_begin());
+ Value *StoredDirectArg = findStoredValue(DirectCI->getArgOperand(2));
+ EXPECT_EQ(StoredDirectArg, F->arg_begin());
}
TEST_F(OpenMPIRBuilderTest, ParallelCancelBarrier) {
@@ -829,6 +850,85 @@
}
}
+TEST_F(OpenMPIRBuilderTest, ParallelForwardAsPointers) {
+ OpenMPIRBuilder OMPBuilder(*M);
+ OMPBuilder.initialize();
+ F->setName("func");
+ IRBuilder<> Builder(BB);
+ OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+ using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+
+ Type *I32Ty = Type::getInt32Ty(M->getContext());
+ Type *I32PtrTy = Type::getInt32PtrTy(M->getContext());
+ Type *StructTy = StructType::get(I32Ty, I32PtrTy);
+ Type *StructPtrTy = StructTy->getPointerTo();
+ Type *VoidTy = Type::getVoidTy(M->getContext());
+ FunctionCallee RetI32Func = M->getOrInsertFunction("ret_i32", I32Ty);
+ FunctionCallee TakeI32Func =
+ M->getOrInsertFunction("take_i32", VoidTy, I32Ty);
+ FunctionCallee RetI32PtrFunc = M->getOrInsertFunction("ret_i32ptr", I32PtrTy);
+ FunctionCallee TakeI32PtrFunc =
+ M->getOrInsertFunction("take_i32ptr", VoidTy, I32PtrTy);
+ FunctionCallee RetStructFunc = M->getOrInsertFunction("ret_struct", StructTy);
+ FunctionCallee TakeStructFunc =
+ M->getOrInsertFunction("take_struct", VoidTy, StructTy);
+ FunctionCallee RetStructPtrFunc =
+ M->getOrInsertFunction("ret_structptr", StructPtrTy);
+ FunctionCallee TakeStructPtrFunc =
+ M->getOrInsertFunction("take_structPtr", VoidTy, StructPtrTy);
+ Value *I32Val = Builder.CreateCall(RetI32Func);
+ Value *I32PtrVal = Builder.CreateCall(RetI32PtrFunc);
+ Value *StructVal = Builder.CreateCall(RetStructFunc);
+ Value *StructPtrVal = Builder.CreateCall(RetStructPtrFunc);
+
+ Instruction *Internal;
+ auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+ BasicBlock &ContinuationBB) {
+IRBuilder<>::InsertPointGuard Guard(Builder);
+Builder.restoreIP(CodeGenIP);
+Internal = Builder.CreateCall(TakeI32Func, I32Val);
+Builder.CreateCall(TakeI32PtrFunc, I32PtrVal);
+Builder.CreateCall(TakeStructFunc, StructVal);
+Builder.CreateCall(TakeStructPtrFunc, StructPtrVal);
+ };
+ auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+Value &VPtr, Value *&ReplacementValue) {
+ReplacementValue = &VPtr;
+return CodeGenIP;
+ };
+ auto FiniCB = [](InsertPointTy) {};
+
+ IRBuilder<>::InsertPoint AllocaIP(&F->getEntryBlock(),
+F->getEntryBlock().getFirstInsertion
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
ftynse added inline comments.
Comment at: clang/test/OpenMP/parallel_codegen.cpp:139
// CHECK: call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*,
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})*
[[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]],
i{{64|32}} %{{.+}})
-// IRBUILDER: call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*,
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})*
[[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]],
i{{64|32}} %{{.+}})
+// IRBUILDER: call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*,
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i{{64|32}}*, i8***)*
[[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i{{64|32}}* %{{.+}}, i8***
[[ARGC_ADDR]])
// ALL: ret i32 0
The order of arguments changes here because we create a use of the
promoted-to-pointer argument before any other uses in the body and the outliner
finds it first. This should be fine because it's just an internal outlined
function that the builder created and the calls to it are emitted accordingly
and in the same builder. I can add a comment that explains this if desired.
If we go with Michael's suggestion not to turn into pointers the integer values
whose size is equal to or smaller than pointer size, this change will not be
necessary. I seem to remember seeing some documentation that says that trailing
arguments to `fork_call` should be _pointers_, but I can't find it anymore.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92189/new/
https://reviews.llvm.org/D92189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
ftynse planned changes to this revision. ftynse added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:736 "Expected copy/create callback to set replacement value!"); - if (ReplacementValue == &V) -return; } jdoerfert wrote: > ftynse wrote: > > jdoerfert wrote: > > > jdoerfert wrote: > > > > I was expecting the above logic to be placed here, after the `PrivCB` > > > > callback as I assumed we would privatize in the sequential part. Clang > > > > does not and we do not either (for now), which is unfortunate. It does > > > > duplicate the privatization and makes this patch more complicated than > > > > I anticipated. I though we can simply replace `ReplacementValue` by > > > > `Reloaded` if `ReplacementValue` wasn't a pointer and therefor put in > > > > an alloca. The fact that we need to reload `V` and then "replace twice" > > > > seems really unfortunate. > > > > > > > > What would happen if you do not have `Reloaded` at all but instead make > > > > it `V = createlodoad(alloca)`? After all, `Reloaded` is equivalent to > > > > `V` in all aspects, right? Would this work? Would we still need the > > > > code below? I feel like there must be a minimal solution as we only > > > > want to replace the value once on the other side of the call edge. > > > -"duplicate privatization" +"duplicate replace all uses with" > > I am not sure I fully follow what you suggest, so I will elaborate on the > > two versions I had considered. > > > > 1. Move the code that loads back the value (currently lines 709-725) after > > this line. This will not remove the need for two separate "replace all > > uses with": there uses of the original `V` in the body that need to be > > replaced with `ReplacementValue`, and there are uses of `V` that could have > > been introduced by `PrivCB` for the purpose of //defining// > > `ReplacementValue` which should be replaced with `Reloaded` instead. This > > doesn't look like it would address your concern about having double > > replacement. > > > > 2. I can keep the code that loads back the value in its current place and > > pass either `V` or `*Reloaded` to `PrivCB`. This will make sure any > > instructions created in `PrivCB` use the `Reloaded` and don't need to be > > update via "replace all uses with" pattern. However, this exposes the > > pointer mechanism to the caller of `CreateParallel`. The `Value &` that > > `PrivCB` receives is not necessarily a value that exists in the user code, > > it can be the alloca we constructed in builder. So the implementation of > > `PrivCB` needs to be aware of it and can no longer rely on, e.g., keeping a > > list of values/instructions that need privatization or directly rely on > > types since the type would change. I decided that I did not want to change > > the contract that `PrivCB` has with `CreateParallel` because I was not > > aware of all its use cases (and have a preference for more "isolated" APIs) > > . However, if you think it is acceptable for the builder in order to reduce > > the complexity/overhead of the code, I can adapt. > > > > There is a flavor of (2) that changes `PrivCB` to take both `V` and > > `Replacement` so that the implementation of `PrivCB` can easily detect when > > the pointer mechanism is active. > Appreciate the detailed explanations! > > > There is a flavor of (2) that changes PrivCB to take both V and Replacement > > so that the implementation of PrivCB can easily detect when the pointer > > mechanism is active. > > This looks like a sweet-spot. We can avoid adding more replacement logic but > the `PrivCB` is aware of what is going on. If you are OK with this solution > as well I'd prefer it. > Great, will do this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92189/new/ https://reviews.llvm.org/D92189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
ftynse updated this revision to Diff 308698.
ftynse added a comment.
Simplify the code by adapting the PrivatizationCallback signature.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92189/new/
https://reviews.llvm.org/D92189
Files:
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/test/OpenMP/parallel_codegen.cpp
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -60,6 +60,25 @@
DebugLoc DL;
};
+// Returns the value stored in the given allocation. Returns null if the given
+// value is not a result of an allocation, if no value is stored or if there is
+// more than one store.
+static Value *findStoredValue(Value *AllocaValue) {
+ Instruction *Alloca = dyn_cast(AllocaValue);
+ if (!Alloca)
+return nullptr;
+ StoreInst *Store = nullptr;
+ for (Use &U : Alloca->uses()) {
+if (auto *CandidateStore = dyn_cast(U.getUser())) {
+ EXPECT_EQ(Store, nullptr);
+ Store = CandidateStore;
+}
+ }
+ if (!Store)
+return nullptr;
+ return Store->getValueOperand();
+};
+
TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
OpenMPIRBuilder OMPBuilder(*M);
OMPBuilder.initialize();
@@ -341,20 +360,25 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
++NumPrivatizedVars;
-if (!isa(VPtr)) {
- EXPECT_EQ(&VPtr, F->arg_begin());
- ReplacementValue = &VPtr;
+if (!isa(Orig)) {
+ EXPECT_EQ(&Orig, F->arg_begin());
+ ReplacementValue = &Inner;
return CodeGenIP;
}
+// Since the original value is an allocation, it has a pointer type and
+// therefore no additional wrapping should happen.
+EXPECT_EQ(&Orig, &Inner);
+
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -401,7 +425,7 @@
EXPECT_EQ(ForkCI->getArgOperand(1),
ConstantInt::get(Type::getInt32Ty(Ctx), 1U));
EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
- EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
+ EXPECT_EQ(findStoredValue(ForkCI->getArgOperand(3)), F->arg_begin());
}
TEST_F(OpenMPIRBuilderTest, ParallelNested) {
@@ -423,12 +447,13 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -515,12 +540,13 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -6
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
ftynse updated this revision to Diff 308933.
ftynse marked 6 inline comments as done.
ftynse added a comment.
Address all remaining review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92189/new/
https://reviews.llvm.org/D92189
Files:
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/test/OpenMP/parallel_codegen.cpp
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -60,6 +60,25 @@
DebugLoc DL;
};
+// Returns the value stored in the given allocation. Returns null if the given
+// value is not a result of an allocation, if no value is stored or if there is
+// more than one store.
+static Value *findStoredValue(Value *AllocaValue) {
+ Instruction *Alloca = dyn_cast(AllocaValue);
+ if (!Alloca)
+return nullptr;
+ StoreInst *Store = nullptr;
+ for (Use &U : Alloca->uses()) {
+if (auto *CandidateStore = dyn_cast(U.getUser())) {
+ EXPECT_EQ(Store, nullptr);
+ Store = CandidateStore;
+}
+ }
+ if (!Store)
+return nullptr;
+ return Store->getValueOperand();
+};
+
TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
OpenMPIRBuilder OMPBuilder(*M);
OMPBuilder.initialize();
@@ -341,20 +360,25 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
++NumPrivatizedVars;
-if (!isa(VPtr)) {
- EXPECT_EQ(&VPtr, F->arg_begin());
- ReplacementValue = &VPtr;
+if (!isa(Orig)) {
+ EXPECT_EQ(&Orig, F->arg_begin());
+ ReplacementValue = &Inner;
return CodeGenIP;
}
+// Since the original value is an allocation, it has a pointer type and
+// therefore no additional wrapping should happen.
+EXPECT_EQ(&Orig, &Inner);
+
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -401,7 +425,7 @@
EXPECT_EQ(ForkCI->getArgOperand(1),
ConstantInt::get(Type::getInt32Ty(Ctx), 1U));
EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
- EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
+ EXPECT_EQ(findStoredValue(ForkCI->getArgOperand(3)), F->arg_begin());
}
TEST_F(OpenMPIRBuilderTest, ParallelNested) {
@@ -423,12 +447,13 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -515,12 +540,13 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return Cod
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
ftynse updated this revision to Diff 308938.
ftynse added a comment.
Herald added subscribers: teijeong, rdzhabarov, tatianashp, msifontes, jurahul,
Kayjukh, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik, lucyrfox,
mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, rriddle,
mehdi_amini.
Herald added a project: MLIR.
Fix a runaway MLIR use of createParallel
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92189/new/
https://reviews.llvm.org/D92189
Files:
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/test/OpenMP/parallel_codegen.cpp
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
Index: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -433,7 +433,7 @@
// attribute (shared, private, firstprivate, ...) of variables.
// Currently defaults to shared.
auto privCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP,
-llvm::Value &vPtr,
+llvm::Value &, llvm::Value &vPtr,
llvm::Value *&replacementValue) -> InsertPointTy {
replacementValue = &vPtr;
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -60,6 +60,25 @@
DebugLoc DL;
};
+// Returns the value stored in the given allocation. Returns null if the given
+// value is not a result of an allocation, if no value is stored or if there is
+// more than one store.
+static Value *findStoredValue(Value *AllocaValue) {
+ Instruction *Alloca = dyn_cast(AllocaValue);
+ if (!Alloca)
+return nullptr;
+ StoreInst *Store = nullptr;
+ for (Use &U : Alloca->uses()) {
+if (auto *CandidateStore = dyn_cast(U.getUser())) {
+ EXPECT_EQ(Store, nullptr);
+ Store = CandidateStore;
+}
+ }
+ if (!Store)
+return nullptr;
+ return Store->getValueOperand();
+};
+
TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
OpenMPIRBuilder OMPBuilder(*M);
OMPBuilder.initialize();
@@ -341,20 +360,25 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
++NumPrivatizedVars;
-if (!isa(VPtr)) {
- EXPECT_EQ(&VPtr, F->arg_begin());
- ReplacementValue = &VPtr;
+if (!isa(Orig)) {
+ EXPECT_EQ(&Orig, F->arg_begin());
+ ReplacementValue = &Inner;
return CodeGenIP;
}
+// Since the original value is an allocation, it has a pointer type and
+// therefore no additional wrapping should happen.
+EXPECT_EQ(&Orig, &Inner);
+
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -401,7 +425,7 @@
EXPECT_EQ(ForkCI->getArgOperand(1),
ConstantInt::get(Type::getInt32Ty(Ctx), 1U));
EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
- EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
+ EXPECT_EQ(findStoredValue(ForkCI->getArgOperand(3)), F->arg_begin());
}
TEST_F(OpenMPIRBuilderTest, ParallelNested) {
@@ -423,12 +447,13 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGen
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
ftynse added a comment. Windows failure is unrelated, broken by an earlier commit to MLIR PDL. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92189/new/ https://reviews.llvm.org/D92189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG240dd92432eb: [OpenMPIRBuilder] forward arguments as
pointers to outlined function (authored by ftynse).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92189/new/
https://reviews.llvm.org/D92189
Files:
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/test/OpenMP/parallel_codegen.cpp
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
Index: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -433,7 +433,7 @@
// attribute (shared, private, firstprivate, ...) of variables.
// Currently defaults to shared.
auto privCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP,
-llvm::Value &vPtr,
+llvm::Value &, llvm::Value &vPtr,
llvm::Value *&replacementValue) -> InsertPointTy {
replacementValue = &vPtr;
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -60,6 +60,25 @@
DebugLoc DL;
};
+// Returns the value stored in the given allocation. Returns null if the given
+// value is not a result of an allocation, if no value is stored or if there is
+// more than one store.
+static Value *findStoredValue(Value *AllocaValue) {
+ Instruction *Alloca = dyn_cast(AllocaValue);
+ if (!Alloca)
+return nullptr;
+ StoreInst *Store = nullptr;
+ for (Use &U : Alloca->uses()) {
+if (auto *CandidateStore = dyn_cast(U.getUser())) {
+ EXPECT_EQ(Store, nullptr);
+ Store = CandidateStore;
+}
+ }
+ if (!Store)
+return nullptr;
+ return Store->getValueOperand();
+};
+
TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
OpenMPIRBuilder OMPBuilder(*M);
OMPBuilder.initialize();
@@ -341,20 +360,25 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
++NumPrivatizedVars;
-if (!isa(VPtr)) {
- EXPECT_EQ(&VPtr, F->arg_begin());
- ReplacementValue = &VPtr;
+if (!isa(Orig)) {
+ EXPECT_EQ(&Orig, F->arg_begin());
+ ReplacementValue = &Inner;
return CodeGenIP;
}
+// Since the original value is an allocation, it has a pointer type and
+// therefore no additional wrapping should happen.
+EXPECT_EQ(&Orig, &Inner);
+
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -401,7 +425,7 @@
EXPECT_EQ(ForkCI->getArgOperand(1),
ConstantInt::get(Type::getInt32Ty(Ctx), 1U));
EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
- EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
+ EXPECT_EQ(findStoredValue(ForkCI->getArgOperand(3)), F->arg_begin());
}
TEST_F(OpenMPIRBuilderTest, ParallelNested) {
@@ -423,12 +447,13 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+Value &Orig, Value &Inner,
+Value *&ReplacementValue) -> InsertPointTy {
// Trivial copy (=firstprivate).
Builder.restoreIP(AllocaIP);
-Type *VTy = VPtr.getType()->getPointerElementType();
-Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
-ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
+Type *VTy = Inner.getType()->getPointerElementType();
+Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
+ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
Builder.restoreIP(CodeGenIP);
Builder.CreateStore(V, ReplacementValue);
return CodeGenIP;
@@ -515,12 +540,13 @@
};
auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-Value &VP
[PATCH] D107540: [OMPIRBuilder] Clarify CanonicalLoopInfo. NFC.
ftynse accepted this revision. ftynse added a comment. This revision is now accepted and ready to land. Thanks! Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:364 + /// over all chunks that are executed on the same thread. Returning + /// CanonicalLoopInfo objects representing then may eventually be useful for + /// the apply clause planned in OpenMP 6.0, but currently whether these are Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:393 + /// \param DL Debug location for instructions added for the + ///workshare-loop construct itself. /// \param CLI A descriptor of the canonical loop to workshare. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:420 + /// \param DL Debug location for instructions added for the + ///workshare-loop construct itself. /// \param CLI A descriptor of the canonical loop to workshare. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1287 +/// The loop is thought to start at PreheaderIP (at the Preheader's terminator, +/// including) and at AfterIP (at the After's first instruction, excluding). +/// That is, instructions in the Preheader and After blocks (except the Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1315 +/// basic blocks. After invalidation, the CanonicalLoopInfo must not be used +/// anymore as its underlaying control flow does not exist anymore. +/// Loop-transformation methods such as tileLoops, collapseLoops and unrollLoop I suppose it may still exist in some cases. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1323 +/// modified loop. What is done is an implementation detail of +/// transformation-implementing method and callers should always assume the the +/// CanonicalLoopInfo passed to it is invalidated and a new object is returned. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1330 +/// Generally, methods consuming CanonicalLoopInfo do not need an +/// OpenMPIRBuilder::InsertPointTy as argument, but uses the locations of the +/// CanonicalLoopInfo to insert new or modify existing instructions. Unless Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107540/new/ https://reviews.llvm.org/D107540 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D136746: [mlir] Saturation arithmetic intrinsics
ftynse added inline comments. Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td:179 +def LLVM_SAddSat : LLVM_BinarySameArgsIntrOpI<"sadd.sat">; +def LLVM_SAddSat : LLVM_BinarySameArgsIntrOpI<"uadd.sat">; +def LLVM_SAddSat : LLVM_BinarySameArgsIntrOpI<"ssub.sat">; gysit wrote: > Ah I missed that one. The Op names need to differ of course LLVM_SAddSat, > LLVM_UAddSat, etc. Indeed, this is breaking the tests. Comment at: mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir:809-816 +// CHECK-DAG: declare i32 @llvm.sadd.sat.i32(i32, i32) #0 +// CHECK-DAG: declare <8 x i32> @llvm.sadd.sat.v8i32(<8 x i32>, <8 x i32>) #0 +// CHECK-DAG: declare i32 @llvm.uadd.sat.i32(i32, i32) #0 +// CHECK-DAG: declare <8 x i32> @llvm.uadd.sat.v8i32(<8 x i32>, <8 x i32>) #0 +// CHECK-DAG: declare i32 @llvm.ssub.sat.i32(i32, i32) #0 +// CHECK-DAG: declare <8 x i32> @llvm.ssub.sat.v8i32(<8 x i32>, <8 x i32>) #0 +// CHECK-DAG: declare i32 @llvm.usub.sat.i32(i32, i32) #0 Nit: I'd rather remove the `#0` matadata because the `0` is transient. I see some cases above also match for that, but it is a mistake that makes tests fragile. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136746/new/ https://reviews.llvm.org/D136746 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D136746: [mlir] Saturation arithmetic intrinsics
ftynse accepted this revision. ftynse added a comment. This revision is now accepted and ready to land. Please wait for the tests to pass before submitting. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136746/new/ https://reviews.llvm.org/D136746 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D134425: [NFC] Create a AllocLikeOpInterface and make memref::AllocOp, memref::AllocaOp and gpu::AllocOp implement it.
ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.
I would expect a new top-level interface (in lib/Interfaces) to go through the
RFC process.
I have concerns about the interface design. Specifically, it appears to be
promoting details specific to operations from one dialect (`memref.alloc(a)`)
such as the fact that the result of an allocation is a memref (it's not, we
have allocations in LLVM and SPIR-V that produce different types), that it has
alignment defined as integer, and that it has some symbol operands presumably
related to memref's affine layouts. If this were to live in `lib/Interfaces`,
it should make either cover wider range of allocation-like operations or make a
clear case as to why this is not desired accompanied by a name change.
Comment at: mlir/include/mlir/Dialect/GPU/IR/GPUOps.td:937-954
+
+/// Returns the dynamic sizes for this alloc operation if specified.
+operand_range getDynamicSizes() { return dynamicSizes(); }
+
+/// Returns the symbol operands for this alloc operation if specified.
+operand_range getSymbolOperands() { return symbolOperands(); }
+
I suppose these are only necessary because the dialect doesn't emit accessor
using the prefixed form? Make sure to rebase this commit because the GPU
dialect must have been switched to the prefixed form -
https://discourse.llvm.org/t/psa-raw-accessors-are-being-removed/65629.
Comment at: mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td:103-120
+
+/// Returns the dynamic sizes for this alloc operation if specified.
+operand_range getDynamicSizes() { return dynamicSizes(); }
+
+/// Returns the symbol operands for this alloc operation if specified.
+operand_range getSymbolOperands() { return symbolOperands(); }
+
Ditto.
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.h:1
+//===- AllocLikeOpInterface.h - alloc like operations interface-===//
+//
Please use the correct header line.
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:1
+//===-- AllocLikeOpInterface.td -*- tablegen
-*===//
+//
Please follow the (implicit) convention from other files for this line: the
file name is left aligned, the file type is right aligned, and the entire line
fits into 80 cols.
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:9
+//
+// Defines the interface for copy-like operations.
+//
This looks wrong.
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:20
+ let description = [{
+A alloc-like operation is one that allocates memory of a given type.
+ }];
Type is not mandatory for allocations, can be just bytes.
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:26
+InterfaceMethod<
+ /*desc=*/"Returns the dynamic dimension sizes of the resultant memref.",
+ /*retTy=*/"::llvm::SmallVector<::mlir::Value>",
Memref is not a mandatory concept for allocating memory, I would rather not
enshrine it in the top-level interface.
Comment at: mlir/include/mlir/Interfaces/AllocLikeOpInterface.td:27
+ /*desc=*/"Returns the dynamic dimension sizes of the resultant memref.",
+ /*retTy=*/"::llvm::SmallVector<::mlir::Value>",
+ /*methodName=*/"getDynamicSizes"
Does it have to be a vector (copy) of values? Can't it be an `OperandRange`, a
`ValueRange` or something similar?
Comment at: mlir/lib/Interfaces/AllocLikeOpInterface.cpp:1
+//===- AllocLikeOpInterface.cpp - Alloc like operations interface in MLIR-===//
+//
Nit: 80 cols.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134425/new/
https://reviews.llvm.org/D134425
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128049: [mlir] move SCF headers to SCF/{IR,Transforms} respectively
ftynse marked an inline comment as done. ftynse added inline comments. Comment at: mlir/include/mlir/Dialect/SCF/Transforms/Transforms.h:13 -#ifndef MLIR_DIALECT_SCF_TRANSFORMS_H_ -#define MLIR_DIALECT_SCF_TRANSFORMS_H_ +#ifndef MLIR_DIALECT_SCF_TRANSFORMS_TRANSFORMS_H_ +#define MLIR_DIALECT_SCF_TRANSFORMS_TRANSFORMS_H_ jpienaar wrote: > Transforms transforms feels a bit strange, for many others I believe this > would have been passes file (which is also not that accurate, patterns and > passes would be more, but most others it is just passes and convenient > shorthand). Keeping the move mostly mechanical is good though There's already Transforms/Passes.h and Transforms/Patterns.h, these things are more like standalone transform functions. Maybe Transforms/Utils.h? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128049/new/ https://reviews.llvm.org/D128049 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128049: [mlir] move SCF headers to SCF/{IR,Transforms} respectively
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b68da2c7d97: [mlir] move SCF headers to SCF/{IR,Transforms}
respectively (authored by ftynse).
Changed prior to commit:
https://reviews.llvm.org/D128049?vs=437955&id=438286#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128049/new/
https://reviews.llvm.org/D128049
Files:
flang/lib/Optimizer/Transforms/AffineDemotion.cpp
flang/lib/Optimizer/Transforms/AffinePromotion.cpp
flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp
mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp
mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp
mlir/include/mlir/Dialect/Async/Transforms.h
mlir/include/mlir/Dialect/Linalg/Utils/Utils.h
mlir/include/mlir/Dialect/SCF/BufferizableOpInterfaceImpl.h
mlir/include/mlir/Dialect/SCF/CMakeLists.txt
mlir/include/mlir/Dialect/SCF/IR/CMakeLists.txt
mlir/include/mlir/Dialect/SCF/IR/SCF.h
mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
mlir/include/mlir/Dialect/SCF/Passes.h
mlir/include/mlir/Dialect/SCF/Passes.td
mlir/include/mlir/Dialect/SCF/Patterns.h
mlir/include/mlir/Dialect/SCF/SCF.h
mlir/include/mlir/Dialect/SCF/SCFOps.td
mlir/include/mlir/Dialect/SCF/TileUsingInterface.h
mlir/include/mlir/Dialect/SCF/Transforms.h
mlir/include/mlir/Dialect/SCF/Transforms/BufferizableOpInterfaceImpl.h
mlir/include/mlir/Dialect/SCF/Transforms/CMakeLists.txt
mlir/include/mlir/Dialect/SCF/Transforms/Passes.h
mlir/include/mlir/Dialect/SCF/Transforms/Passes.td
mlir/include/mlir/Dialect/SCF/Transforms/Patterns.h
mlir/include/mlir/Dialect/SCF/Transforms/TileUsingInterface.h
mlir/include/mlir/Dialect/SCF/Transforms/Transforms.h
mlir/include/mlir/Dialect/SCF/Utils/Utils.h
mlir/include/mlir/InitAllDialects.h
mlir/include/mlir/InitAllPasses.h
mlir/lib/CAPI/Dialect/SCF.cpp
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
mlir/lib/Conversion/LinalgToLLVM/LinalgToLLVM.cpp
mlir/lib/Conversion/LinalgToStandard/LinalgToStandard.cpp
mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
mlir/lib/Conversion/SCFToControlFlow/SCFToControlFlow.cpp
mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp
mlir/lib/Conversion/SCFToGPU/SCFToGPUPass.cpp
mlir/lib/Conversion/SCFToOpenMP/SCFToOpenMP.cpp
mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRVPass.cpp
mlir/lib/Conversion/ShapeToStandard/ConvertShapeConstraints.cpp
mlir/lib/Conversion/ShapeToStandard/ShapeToStandard.cpp
mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamedPass.cpp
mlir/lib/Conversion/TosaToLinalg/TosaToLinalgPass.cpp
mlir/lib/Conversion/TosaToSCF/TosaToSCF.cpp
mlir/lib/Conversion/TosaToSCF/TosaToSCFPass.cpp
mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp
mlir/lib/Dialect/Affine/Transforms/LoopCoalescing.cpp
mlir/lib/Dialect/Affine/Utils/LoopUtils.cpp
mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp
mlir/lib/Dialect/Func/Transforms/PassDetail.h
mlir/lib/Dialect/GPU/Transforms/MemoryPromotion.cpp
mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
mlir/lib/Dialect/Linalg/Transforms/CodegenStrategy.cpp
mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
mlir/lib/Dialect/Linalg/Transforms/LinalgStrategyPasses.cpp
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
mlir/lib/Dialect/SCF/CMakeLists.txt
mlir/lib/Dialect/SCF/IR/CMakeLists.txt
mlir/lib/Dialect/SCF/IR/SCF.cpp
mlir/lib/Dialect/SCF/SCF.cpp
mlir/lib/Dialect/SCF/TransformOps/SCFTransformOps.cpp
mlir/lib/Dialect/SCF/Transforms/BufferizableOpInterfaceImpl.cpp
mlir/lib/Dialect/SCF/Transforms/Bufferize.cpp
mlir/lib/Dialect/SCF/Transforms/ForToWhile.cpp
mlir/lib/Dialect/SCF/Transforms/LoopCanonicalization.cpp
mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp
mlir/lib/Dialect/SCF/Transforms/LoopRangeFolding.cpp
mlir/lib/Dialect/SCF/Transforms/LoopSpecialization.cpp
mlir/lib/Dialect/SCF/Transforms/ParallelLoopCollapsing.cpp
mlir/lib/Dialect/SCF/Transforms/ParallelLoopFusion.cpp
mlir/lib/Dialect/SCF/Transforms/ParallelLoopTiling.cpp
mlir/lib/Dialect/SCF/Transforms/PassDetail.h
mlir/lib/Dialect/SCF/Transforms/StructuralTypeConversions.cpp
mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
mlir/lib/Dialect/SCF/Utils/AffineCanonicalizationUtils.cpp
mlir/lib/Dialect/SCF/Utils/Utils.cpp
mlir/lib/Dialect/SparseT
[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic
ftynse added inline comments. Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:367 // that can be valid on the real entry. - // This is what I want to do AttributeList NewAttrs = AttributeList::get(Ctx, AttributeList::FunctionIndex, This cleanup should go in a separate commit. Comment at: mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp:130 - llvm::CallInst *inst = builder.CreateCall(fn, moduleTranslation.lookupValues(op.getOperands())); Don't use `auto` unless the type is obvious from context (e.g., the RHS is a cast) or difficult to spell. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151492/new/ https://reviews.llvm.org/D151492 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151492: Add fastmath attributes to llvm.call_intrinsic
ftynse added inline comments. Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:367 // that can be valid on the real entry. - // This is what I want to do AttributeList NewAttrs = AttributeList::get(Ctx, AttributeList::FunctionIndex, electriclilies wrote: > ftynse wrote: > > This cleanup should go in a separate commit. > For some reason arcanist is showing the diff between my 2 latest commits, not > between main and my branch, I added this in a previous commit then removed it > in my final clean up commit.. If you create it with `arc`, the command indicates with respect to which other commit the diff is taken, e.g. `arc diff HEAD^` is the diff between the currently checked out commit and the previous one (`HEAD^`). You can do several steps (`HEAD^^^` for three) or specify the commit hash explicitly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151492/new/ https://reviews.llvm.org/D151492 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.
ftynse added inline comments. Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h:13 + +#ifndef MLIR_DIALECT_UTILS_H +#define MLIR_DIALECT_UTILS_H This tag is wrong. It should be `MLIR_TARGET_LLVMIR_DIALECT_UTILS_H`. Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h:59 +#endif // MLIR_DIALECT_UTILS_H \ No newline at end of file Nit: please add a newline. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142914/new/ https://reviews.llvm.org/D142914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.
ftynse added inline comments. Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h:27 +/// Create a constant string location from the MLIR Location information. +static llvm::Constant * +createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder &builder, It's not great to have static functions in a header. I suppose this is done to avoid build dependencies, but it's better to get the dependencies right. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142914/new/ https://reviews.llvm.org/D142914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
