[SandboxIR] Added isVolatile args to existing LoadInst::create function #100850

wizardengineer · 2024-07-27T01:57:57Z

Added isVolatile args along with the tests

Previous PR: #100781

github-actions · 2024-07-27T01:59:42Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

llvm/include/llvm/SandboxIR/SandboxIR.h

github-actions · 2024-07-27T02:01:43Z

✅ With the latest revision this PR passed the C/C++ code formatter.

vporpo · 2024-07-27T05:47:36Z

Try using clang-format to fix the formatting issues.

llvm/unittests/SandboxIR/SandboxIRTest.cpp

llvm/lib/SandboxIR/SandboxIR.cpp

llvm/include/llvm/SandboxIR/SandboxIR.h

llvm/lib/SandboxIR/SandboxIR.cpp

llvm/unittests/SandboxIR/SandboxIRTest.cpp

wizardengineer · 2024-07-28T02:54:41Z

@vporpo Is there anything else I need to change? If not, is it okay for you to merge this?

vporpo · 2024-07-28T03:52:28Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

@@ -764,11 +764,21 @@ define void @foo(ptr %arg0, ptr %arg1) {
  // Check create(InsertBefore)
  sandboxir::LoadInst *NewLd =
      sandboxir::LoadInst::create(Ld->getType(), Arg1, Align(8),
-                                  /*InsertBefore=*/Ret, Ctx, "NewLd");
+                                  /*InsertBefore=*/Ret, Ctx,
+                                  /*IsVolatile=*/false, "NewLd");


This is OK for this PR, but having to specify the IsVolatile argument whenever you create non-volatile LoadInsts looks a bit too verbose. The problem is that most of the times you will be creating an instruction with a name, so you can't take advantage of the default value f the IsVolatile=false argument. And swapping IsVolatile with Name isn't what most users will expect.

If you have time, try creating a follow-up PR with two additional create() functions that don't have the IsVolatile argument and remove the default =false value for these ones. So the functions would be:

1. create(Type *Ty, Value *Ptr, MaybeAlign Align, Instruction *InsertBefore, Context &Ctx, bool IsVolatile, const Twine &Name = "") 2. create(Type *Ty, Value *Ptr, MaybeAlign Align, Instruction *InsertBefore, Context &Ctx, const Twine &Name = "") 3. create(Type *Ty, Value *Ptr, MaybeAlign Align, BasicBlock *InsertAtEnd, Context &Ctx, bool IsVolatile, const Twine &Name = "") 4. create(Type *Ty, Value *Ptr, MaybeAlign Align, BasicBlock *InsertAtEnd, Context &Ctx, const Twine &Name = "")

Then in the body of 2 you would just call 1 with /*IsVolatile=*/false, and similarly in the body of 4 you would call 3.

Completely understandable in terms of LoadInst looking verbose. Btw, do you still want the bool IsVolatile to be before the Context &Ctx in the follow-up PR?

Yes, that would be even better.

Thank you for the suggestion, I'll start on the follow-up PR.

This is OK for this PR

I was a bit confuse on this. Were you suggesting that this PR will get merge first before creating the follow-up NFC PR? If not, then I'll start on the follow-up.

Yes, let's merge this first and then you can work on top of it.

llvm/unittests/SandboxIR/SandboxIRTest.cpp

vporpo

LG

llvm-ci · 2024-07-28T20:48:54Z

LLVM Buildbot has detected a new failure on builder openmp-offload-libc-amdgpu-runtime running on omp-vega20-1 while building llvm at step 10 "Add check check-offload".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/2780

Here is the relevant piece of the build log for the reference:

Step 10 (Add check check-offload) failure: test (failure)
******************** TEST 'libomptarget :: amdgcn-amd-amdhsa :: offloading/schedule.c' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
# RUN: at line 2
/home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./bin/clang -fopenmp    -I /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test -I /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -L /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -L /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./lib -L /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -nogpulib -Wl,-rpath,/home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -Wl,-rpath,/home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -Wl,-rpath,/home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./lib  -fopenmp-targets=amdgcn-amd-amdhsa /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test/offloading/schedule.c -o /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/schedule.c.tmp /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./lib/libomptarget.devicertl.a && /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/schedule.c.tmp 2>&1 | /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./bin/FileCheck /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test/offloading/schedule.c
# executed command: /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./bin/clang -fopenmp -I /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test -I /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -L /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -L /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./lib -L /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -nogpulib -Wl,-rpath,/home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -Wl,-rpath,/home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -Wl,-rpath,/home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./lib -fopenmp-targets=amdgcn-amd-amdhsa /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test/offloading/schedule.c -o /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/schedule.c.tmp /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./lib/libomptarget.devicertl.a
# executed command: /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/schedule.c.tmp
# executed command: /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./bin/FileCheck /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test/offloading/schedule.c
# .---command stderr------------
# | /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test/offloading/schedule.c:80:12: error: CHECK: expected string not found in input
# |  // CHECK: test no order OK
# |            ^
# | <stdin>:1:1: note: scanning from here
# | Fail to schedule in order.
# | ^
# | <stdin>:2:1: note: possible intended match here
# | test ordered OK
# | ^
# | 
# | Input file: <stdin>
# | Check file: /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.src/offload/test/offloading/schedule.c
# | 
# | -dump-input=help explains the following input dump.
# | 
# | Input was:
# | <<<<<<
# |             1: Fail to schedule in order. 
# | check:80'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
# |             2: test ordered OK 
# | check:80'0     ~~~~~~~~~~~~~~~~
# | check:80'1     ?                possible intended match
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1

--

********************

wizardengineer · 2024-07-28T21:12:59Z

I looked into this bug, it doesn't seem to be related to this PR. I'm not sure why there's a Buildbot failure?

vporpo · 2024-07-28T22:21:10Z

Don't worry about it, it looks unrelated.

Added isVolatile args in LoadInst::create function

f66ee5e

wizardengineer commented Jul 27, 2024

View reviewed changes

llvm/include/llvm/SandboxIR/SandboxIR.h Outdated Show resolved Hide resolved

vporpo reviewed Jul 27, 2024

View reviewed changes

removed some unneeded code and clang format

7855441

wizardengineer commented Jul 27, 2024

View reviewed changes

llvm/lib/SandboxIR/SandboxIR.cpp Outdated Show resolved Hide resolved

vporpo reviewed Jul 27, 2024

View reviewed changes

small changes

8e8c94f

vporpo reviewed Jul 28, 2024

View reviewed changes

added volatile check for NewLd

f9266b0

vporpo reviewed Jul 28, 2024

View reviewed changes

llvm/unittests/SandboxIR/SandboxIRTest.cpp Outdated Show resolved Hide resolved

simple fix

587b557

vporpo approved these changes Jul 28, 2024

View reviewed changes

vporpo merged commit 4121191 into llvm:main Jul 28, 2024
7 checks passed

wizardengineer deleted the _SandboxIR_Added_isVolatile_args_to_LoadInst_create branch July 28, 2024 23:37

wizardengineer mentioned this pull request Jul 29, 2024

[SandboxIR][NFC] Fixes for LoadInst::create functions #100955

Merged

[SandboxIR] Added isVolatile args to existing LoadInst::create function #100850

[SandboxIR] Added isVolatile args to existing LoadInst::create function #100850

Uh oh!

Conversation

wizardengineer commented Jul 27, 2024

Uh oh!

github-actions bot commented Jul 27, 2024

Uh oh!

Uh oh!

github-actions bot commented Jul 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vporpo commented Jul 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wizardengineer commented Jul 28, 2024

Uh oh!

vporpo Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

wizardengineer Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

vporpo Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

wizardengineer Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

vporpo Jul 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vporpo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Jul 28, 2024

Uh oh!

wizardengineer commented Jul 28, 2024

Uh oh!

vporpo commented Jul 28, 2024

Uh oh!

Uh oh!

github-actions bot commented Jul 27, 2024 •

edited

Loading