[SandboxIR] Add pointer-diff utility function #110176

Sterling-Augustine · 2024-09-26T21:52:15Z

This will be used when gathering seeds to calculate the lane an instruction in the bundle uses.

vporpo · 2024-09-26T21:54:43Z

llvm/include/llvm/SandboxIR/Utils.h

+    return *Diff > 0;
+  }
+
+  /// \Returns the number gap between the memory locations accessed by \p I0 and


"the number gap" did you mean to say "the gap" ?

vporpo · 2024-09-26T21:56:39Z

llvm/include/llvm/SandboxIR/Utils.h

  memoryLocationGetOrNone(const Instruction *I) {
    return llvm::MemoryLocation::getOrNone(cast<llvm::Instruction>(I->Val));
  }
+  /// \Returns true if \p I1 accesses a memory location lower than \p I2.


I think we should also mention that it returns false if we can't determine the diff.

vporpo · 2024-09-26T21:57:14Z

llvm/include/llvm/SandboxIR/Utils.h

  }
+  /// \Returns true if \p I1 accesses a memory location lower than \p I2.
+  template <typename LoadOrStoreT>
+  static bool atLowerAddress(LoadOrStoreT *I0, LoadOrStoreT *I1,


nit: Since this calls getPointerDiffInBytes() I would move the function below it.

vporpo · 2024-09-26T21:58:42Z

llvm/include/llvm/SandboxIR/Utils.h

+  static std::optional<int>
+  getPointerDiffInBytes(LoadOrStoreT *I0, LoadOrStoreT *I1, ScalarEvolution &SE,
+                        const DataLayout &DL) {
+    static_assert(std::is_same<LoadOrStoreT, sandboxir::LoadInst>::value ||


Do we need sandboxir:: here? We are in namespace llvm::sandboxir so it should work without it?

fixed. But leaving the sandboxir:: in the assertion message, because that is how I messed it up in the first place. :-)

vporpo · 2024-09-26T22:00:10Z

llvm/unittests/SandboxIR/UtilsTest.cpp

+  auto *L1 = cast<sandboxir::LoadInst>(&*It++);
+  auto *L2 = cast<sandboxir::LoadInst>(&*It++);
+  auto *L3 = cast<sandboxir::LoadInst>(&*It++);
+  (void)L3;


nit: We are using [[maybe_unused]] in the rest of the tests instead of (void)`.

vporpo · 2024-09-26T22:01:44Z

llvm/unittests/SandboxIR/UtilsTest.cpp

+  EXPECT_EQ(*sandboxir::Utils::getPointerDiffInBytes(L0, V3L1, SE, DL), 4);
+  EXPECT_EQ(*sandboxir::Utils::getPointerDiffInBytes(V2L0, V2L2, SE, DL), 8);
+  EXPECT_EQ(*sandboxir::Utils::getPointerDiffInBytes(V2L0, V2L3, SE, DL), 12);
+  EXPECT_EQ(*sandboxir::Utils::getPointerDiffInBytes(V2L3, V2L0, SE, DL), -12);


How about tests for atLowerAddress() ?

vporpo

LG, thanks!

tschuett · 2024-09-27T05:21:31Z

llvm/include/llvm/SandboxIR/Utils.h

+  static std::optional<int>
+  getPointerDiffInBytes(LoadOrStoreT *I0, LoadOrStoreT *I1, ScalarEvolution &SE,
+                        const DataLayout &DL) {
+    static_assert(std::is_same<LoadOrStoreT, LoadInst>::value ||


You can use std::is_same_v<LoadOrStoreT, LoadInst> to make it briefer.

…ipelining (llvm#110035) This fixes loop iteration count calculation if the step is a negative value, where we should adjust the added delta from `step-1` to `step+1` when doing the ceil div.

This patch implements sandboxir::Module. It provides access to globals.

While merging profiles, some fields in the input header, e.g. HashFunction, could be uninitialized leading to a UMR. Initialize merged header with the first input header. Fixes llvm#109592

…Fallback (llvm#109743) This should fix SANITIZER_FREEBSD and simplify SANITIZER_GLIBC version. Also the PR make readers aware of problematic `ThreadDescriptorSizeFallback` for SANITIZER_FREEBSD. Maybe it will encourage FreeBSD maintainers to improve the functions, or prove that it's not needed at all.

…build (llvm#110038) Summary: The `libc` project automatically adds `libc` to the projects list if it's in the runtimes list. This then causes it to enter the projects directory to bootstrap a handful of utilities, This usage conflicts with a new error message with effectively stopped us from doing this. This patch weakens the error message to permit this single case.

…mber of elements in operands. Patch adds basic support for non-power-of-2 number of elements in operands. The patch still requires that this number addresses whole registers. Reviewers: RKSimon, preames Reviewed By: preames Pull Request: llvm#107273

While printing functions, expand --print-only flag to accept section names. E.g., "--print-only=\.init" will only print functions from ".init" section.

This is mostly for test: under contextual profiling, we perform ICP for those indirect callsites which have targets marked as `alwaysinline`. This helped uncover a bug with the way the profile was updated upon ICP, where we were skipping over the update if the target wasn't called in that context. That was resulting in incorrect counts for the indirect BB. Also flyby fix to the total/direct count values, they should be 64-bit (as all counters are in the contextual profile)

This bug is caused by the BigInt implementation failing to initialize from errno. Explanation below, but the fix is a static cast to int. The bug only shows up on risc-v 32 because of a chain of type-oddities: 1) Errno is provided by a struct with an implicit cast to int. 2) The printf parser uses an int128 to store the value of a conversion on systems with long double greater than double. 3) On systems without native int128 support we use our own BigInt instead. These combine such that if both long double and int128 exist (e.g. on x86) there's no issue, errno is implicitly cast to int, which is extended to int128. If long double is double (e.g. on arm32) then int64 is used in the printf parser, the implicit cast works, and there's no issue. The only way this would come up is if the target has a proper long double type, but not int128, which is the case for at least the current risc-v 32 bot.

The proxy header errno_macros.h should include relative to `libc/` but it was instead including relative to `libc/include/`. This patch fixes this by adding the `include` to the paths.

…110050) The getter functions simply turn around and call the LLVM counterparts. This is fine until we don't add new sandbox IR opcodes. At that point, we may have to explicitly check if the behavior is different.

…9644) Currently the EVL recipes transfer the tail masking to the EVL. But in the legacy cost model, the mask exist and will calculate the instruction cost of the mask. To fix the difference between the VPlan-based cost model and the legacy cost model, we always calculate the instruction cost for the mask in the EVL recipes. Note that we should remove the mask cost in the EVL recipes when we don't need to compare to the legacy cost model. This patch also fixes llvm#109468.

Ensure store to const addrspace is not allowed by Linter.

…. NFC

Add support for !"cluster_dim_{x,y,z}" metadata to allow specifying cluster dimensions on a kernel function in llvm. If any of these metadata entries are present, the `.explicitcluster` PTX directive is used and the specified dimensions are lowered with the `.reqnctapercluster` directive. For more details see: [PTX ISA: 11.7. Cluster Dimension Directives] (https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#cluster-dimension-directives)

…ion/macro to free up a bit slot (llvm#82787) Follow on to llvm#81525 and llvm#81901 in the series of consolidating bits in TSFlags. Remove renamedInGFX9 from SIInstrFormats.td and move to helper function/macro in SIInstrInfo. renamedInGFX9 points to V_{add, sub, subrev, addc, subb, subbrev}_ U32 and V_{div_fixup_F16, fma_F16, interp_p2_F16, mad_F16, mad_U16, mad_I16}.

Summary: If this tool changes it should be rebuilt, as its used in the compilation pipeline.

Quick fix with small performance improvement for getPaddedLen function.

The module in LLVM can have more then one CompilationUnit when e.g. modules are combined by llvm-linker. This property also needs to be handled in DI.

llvm#85325) This fixes a crash when we attempt to instantiate a lambda with an `AnnotatedType`, refactors the code that handles transforming the function type of a lambda, and improves source fidelity for lambda function types. This fixes llvm#85120 and fixes llvm#85154. --------- Co-authored-by: Yuxuan Chen <[email protected]> Co-authored-by: Doug Wyatt <[email protected]>

Removed dependency on `Transforms/Utils` from `CtxProfAnalysis.cpp` - it was unnecessary to begin with.

Sterling-Augustine · 2024-09-27T23:40:14Z

I'm not sure why this is showing so many files that are irrelevant. Somehow a merge problem or something. I'm going to close and retry.

[SandboxIR] Add pointer-diff utility function.

cba50ef

This will be used when gathering seeds to calculate the lane an instruction in the bundle uses.

vporpo reviewed Sep 26, 2024

View reviewed changes

Address comments

c210047

vporpo approved these changes Sep 26, 2024

View reviewed changes

tschuett reviewed Sep 27, 2024

View reviewed changes

sjw36 and others added 25 commits September 27, 2024 16:20

[mlir][scf] Fix loop iteration calculation for negative step in LoopP…

910caea

…ipelining (llvm#110035) This fixes loop iteration count calculation if the step is a negative value, where we should adjust the added delta from `step-1` to `step+1` when doing the ceil div.

[SandboxIR] Implement Module (llvm#109716)

6280035

This patch implements sandboxir::Module. It provides access to globals.

[gn build] Port 7e5df5b

9d15da5

[BOLT][merge-fdata] Initialize YAML profile header (llvm#109613)

031161e

While merging profiles, some fields in the input header, e.g. HashFunction, could be uninitialized leading to a UMR. Initialize merged header with the first input header. Fixes llvm#109592

[BOLT] Allow sections in --print-only flag (llvm#109622)

dc1a266

While printing functions, expand --print-only flag to accept section names. E.g., "--print-only=\.init" will only print functions from ".init" section.

[libc] Fix errno_macros.h include paths. (llvm#110057)

53fa037

The proxy header errno_macros.h should include relative to `libc/` but it was instead including relative to `libc/include/`. This patch fixes this by adding the `include` to the paths.

[Lint][AMDGPU] No store to const addrspace (llvm#109181)

04c90f1

Ensure store to const addrspace is not allowed by Linter.

[RISCV] Reuse Factor variable instead of hardcoding 2 in other places…

c44741e

…. NFC

[SandboxIR] Fix failing unittest introduced by 5103910

131c020

[SandboxIR][NFC] Move Context class into a separate file (llvm#110049)

33b678c

[gn build] Port 165a912

6d98e45

[libc] Fix missing dependency on the nvlink-wrapper (llvm#110056)

c1bd049

Summary: If this tool changes it should be rebuilt, as its used in the compilation pipeline.

[SPIR-V][NFC] More efficient getPaddedLen (llvm#105823)

41398fa

Quick fix with small performance improvement for getPaddedLen function.

[SPIR-V] Support for multiple DebugCompilationUnit in DI (llvm#109645)

996c0f3

The module in LLVM can have more then one CompilationUnit when e.g. modules are combined by llvm-linker. This property also needs to be handled in DI.

[nfc][ctx_prof] Remove unnecessary include

4702aae

Removed dependency on `Transforms/Utils` from `CtxProfAnalysis.cpp` - it was unnecessary to begin with.

Sterling-Augustine requested review from a team, DeinAlptraum, JDevlieghere, aaupov, antiagainst, ayermolo, banach-space, dcaballe, dcci, ftynse, grypp, hanhanW, keith, kuhar, maksfb, nicolasvasilache, nikic, rafaelauler, rengolin and rupprecht as code owners September 27, 2024 23:37

Sterling-Augustine closed this Sep 27, 2024

ldionne removed request for a team, DeinAlptraum, ayermolo, dcaballe and nicolasvasilache September 30, 2024 15:54

Sterling-Augustine deleted the memorder branch October 7, 2024 21:14

[SandboxIR] Add pointer-diff utility function #110176

[SandboxIR] Add pointer-diff utility function #110176

Uh oh!

Conversation

Sterling-Augustine commented Sep 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vporpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sterling-Augustine commented Sep 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

117 participants