Page MenuHomePhabricator

dmilosevic141 (Dimitrije Milošević)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 5 2021, 8:03 AM (46 w, 2 d)

Recent Activity

Mon, Sep 12

dmilosevic141 added a comment to D129749: [Sanitizer][MIPS] Fix stat struct size for the O32 ABI.

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Thanks for pointing this out @raj.khem.

As far as I can see, -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 is expected by default for the O32 ABI in LLVM - see https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/base-config-ix.cmake#L224. GCC, however, doesn't follow this - please see the following discussion https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597617.html.

Rather than working with CMake, shouldn't we be able to just add a check if _LARGEFILE_SOURCE is defined and if _FILE_OFFSET_BITS is defined and equal to 64? If it is so, struct_kernel_stat_sz would be equal to 160, rather than 144 (for the O32 ABI). This change would then be cherry-picked into GCC. Thus we would cover every possible case for both LLVM and GCC.

The problem is we need to ensure libsanitizer functional properly, not just papering over the issue and make it build. That's the point of why we define our own value of struct_kernel_stat_sz and check it with COMPILER_CHECK, instead of just using sizeof(struct stat).

Mon, Sep 12, 8:12 AM · Restricted Project, Restricted Project
dmilosevic141 added a comment to D129749: [Sanitizer][MIPS] Fix stat struct size for the O32 ABI.

I think this patch has created regression when using largefile support. So try adding -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 to compiler cmdline on mips (o32 ABI) and the size of this struct is bumped to 160 on linux/glibc see definition of stat for mips in glibc

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mips/bits/struct_stat.h;h=71594e456244d1e4a746c859048793385c5be376;hb=HEAD#l28

Mon, Sep 12, 1:18 AM · Restricted Project, Restricted Project

Jul 14 2022

dmilosevic141 requested review of D129749: [Sanitizer][MIPS] Fix stat struct size for the O32 ABI.
Jul 14 2022, 4:01 AM · Restricted Project, Restricted Project

Jun 7 2022

dmilosevic141 added inline comments to D127096: [MIPS][AddressSanitizer] Fix the shadow offset hook for the n32 ABI.
Jun 7 2022, 3:03 AM · Restricted Project, Restricted Project
dmilosevic141 updated the diff for D127096: [MIPS][AddressSanitizer] Fix the shadow offset hook for the n32 ABI.

Removed the unnecessary check for the N32 ABI if LongSize is equal to 64.

Jun 7 2022, 3:02 AM · Restricted Project, Restricted Project

Jun 6 2022

dmilosevic141 updated the summary of D127098: [MIPS][AddressSanitizer] Resolve build issues for the n32 ABI.
Jun 6 2022, 11:20 PM · Restricted Project, Restricted Project
dmilosevic141 updated the diff for D127098: [MIPS][AddressSanitizer] Resolve build issues for the n32 ABI.

I've changed the following:

  • Rebased.
  • Made sure MIPS is targeted before checking its ABI.
Jun 6 2022, 7:00 AM · Restricted Project, Restricted Project
dmilosevic141 requested review of D127098: [MIPS][AddressSanitizer] Resolve build issues for the n32 ABI.
Jun 6 2022, 3:58 AM · Restricted Project, Restricted Project
dmilosevic141 requested review of D127096: [MIPS][AddressSanitizer] Fix the shadow offset hook for the n32 ABI.
Jun 6 2022, 3:27 AM · Restricted Project, Restricted Project

May 9 2022

dmilosevic141 updated the diff for D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars.

Thanks @reames!
I took some time to dive deeper into the predicated instructions. So far, two things stand out to me:

  • The ScalarizeMaskedMemIntrin pass, which is a standard part of the CodeGen pipeline. This pass translates the masked memory intrinsics into chains of basic blocks, by loading/storing elements one-by-one. For single-value vectors, handling looks pretty much identical to the initial version of the patch. Scalarization of the masked memory intrinsics is, however, target-dependent (e.g. for x86, the scalarization of calls with single-value vectors passed in as arguments does occur; for RISC-V, it does not).
  • The Vector Predication Roadmap (Vector Predication Roadmap).

Having said that, I'm a little bit stuck in regards to the next steps towards better handling of single-value vectors, as I'm not sure that the ScalarieMaskedMemIntrin pass is *the* place for that. The Vector Predication Roadmap is a little bit overwhelming - i.e. I'm not sure which part of it is related to the better handling of single-value vectors. If you (or anyone else) have something concrete to point me to, I'd be really thankful. :)
I've updated the patch to use the llvm.masked.store intrinsic, instead of the complicated CFG manipulation.

May 9 2022, 2:36 AM · Restricted Project, Restricted Project

Mar 31 2022

dmilosevic141 added inline comments to D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars.
Mar 31 2022, 12:57 AM · Restricted Project, Restricted Project
dmilosevic141 updated the diff for D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars.

Thanks @reames! Sorry for the delayed response, I haven't been able to fully focus on this topic in the past couple of months.

My high level concern here is profitability. It's unclear to me that inserting a flag iv, and the conditional store outside the loop is generally profitable. It's clearly profitable in many cases, but I'm a bit hesitant on whether it makes sense to do as a canonicalization. I'd love to see some data on the impact of this.

Definetly, hopefully I'll be able to provide the data soon.

Structure wise, I strongly encourage you to use predicated stores not CFG manipulation. LICM does not generally modify the CFG; we can, but the analysis updates are complicated. (See the diamond hoisting code.) I generally think that a predicate store instruction is the "right" construct here as it leads to more obvious optimization outcomes.

I haven't come across the predicated store instructions yet. Based on the first look, they definetly fit the needs better than the CFG manipulation, thanks for the directions.
Here's my vision for using them, please let me know your thoughts:

  • Hoisting load instructions into the preheader would still stay the same (using regular load instructions).
  • An additional flag would still be needed. It'd be used (e.g. initialized in the preheader basic block, its value propagated through the basic blocks) the same way.
  • Sinking the conditional store instructions into the exit block(s) with CFG manipulation would be replaced with an appropriate predicated store instructions sunk into the exit block(s).

The summary example would then look like this:

...
entry:
  u.promoted = load u
  br for.cond
for.cond:
  u.flag = phi [ false, entry ], [ u.flag.next, for.inc ]
  inc = phi [ u.promoted, entry ], [ inc.next, for.inc ]
  ...
for.body: 
...
if.then:
  inc.if.then = add inc, 1
  br for.inc
for.inc:
  u.flag.next = phi [ true, if.then ], [ u.flag, for.body ]
  inc.next = phi [ inc.if.then, if.then ], [ inc, for.body ]
  ...
for.cond.cleanup: ; This is the only exit block.
  u.flag.lcssa = phi [ u.flag, for.cond ] ; Get the flag value.
  inc.lcssa = phi [ inc, for.cond ]
  call @llvm.masked.store(<1x<u_type>> inc.lcssa, <1x<u_type>> &u, i32 <u_alignment>, <1xi1> u.flag.lcssa)
  ret void
}
...

I'll also take some more time diving deeper into the predicated instructions.

However, our masked.load handling for scalar (e.g. single value vectors) leaves a bit to be desired. I'd started improving it a bit (with this goal in mind), but if you're serious about this, you'd need to spend some time working through related issues there first.

I'm guessing the same goes for the masked.store.* intrinsics, so

call @llvm.masked.store(<1x<u_type>> inc.lcssa, <1x<u_type>>* &u, i32 <u_alignment>, <1xi1> u.flag.lcssa)

should be swapped with something more appropriate for scalar values?
To summarize, I've rebased and fixed a few things @reames pointed out.

Mar 31 2022, 12:56 AM · Restricted Project, Restricted Project

Dec 29 2021

dmilosevic141 requested review of D116373: [docs] Add documentation for the plot option.
Dec 29 2021, 6:41 AM · Restricted Project

Dec 27 2021

dmilosevic141 added a comment to D116302: [utils][compare.py] Add a plotting option.

I agree, thanks!

Dec 27 2021, 4:51 AM
dmilosevic141 requested review of D116302: [utils][compare.py] Add a plotting option.
Dec 27 2021, 4:03 AM

Dec 7 2021

dmilosevic141 added inline comments to D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars.
Dec 7 2021, 6:36 AM · Restricted Project, Restricted Project
dmilosevic141 updated the diff for D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars.

Thanks @djtodoro !
I've updated the following:

  • Simplified the summary example.
  • Removed the unnecessary comment from the LICM.cpp file, as well as unnecessary attributes and metadata from the conditional-access-promotion.ll file.
Dec 7 2021, 6:30 AM · Restricted Project, Restricted Project
dmilosevic141 updated the summary of D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars.
Dec 7 2021, 5:54 AM · Restricted Project, Restricted Project
dmilosevic141 requested review of D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars.
Dec 7 2021, 5:36 AM · Restricted Project, Restricted Project

Nov 12 2021

dmilosevic141 updated the diff for D113465: [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing.

Ran git-clang-format so that changes made match latest clang-format style.

Nov 12 2021, 1:55 AM · debug-info, Restricted Project
dmilosevic141 updated the diff for D113465: [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing.
  • Rebased.
  • Updated comments within the .ll file. Thanks @djtodoro!
Nov 12 2021, 12:57 AM · debug-info, Restricted Project

Nov 11 2021

dmilosevic141 updated the diff for D113465: [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing.

Removed a trailing whitespace within the .ll file which prevented patch application in pre-merge checks.

Nov 11 2021, 11:32 PM · debug-info, Restricted Project

Nov 10 2021

dmilosevic141 updated the diff for D113465: [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing.

Thanks @Orlando!
As far as the example in the summary goes, it was meant to showcase cross CU referencing in general, hence why the variables in the concrete inlined instance tree had full location coverage. I've updated the summary by adding a concrete example of what it looks like when a variable in a concrete inlined instance tree has 0% location coverage, which is what this patch actually fixes.
I've also dropped the std::unique_ptr wrapper around CrossCUReferencingDIELocationTy as there really was no reason for it.

Nov 10 2021, 5:59 AM · debug-info, Restricted Project

Nov 9 2021

dmilosevic141 updated the diff for D113465: [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing.

Thanks for the suggestions!

Nov 9 2021, 3:54 AM · debug-info, Restricted Project
dmilosevic141 retitled D113465: [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing from [llvm-dwarfdump] Implementation of an add-on for handling LTO cases with CCU referencing to [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing.
Nov 9 2021, 3:16 AM · debug-info, Restricted Project
dmilosevic141 requested review of D113465: [llvm-dwarfdump][Statistics] Handle LTO cases with cross CU referencing.
Nov 9 2021, 12:09 AM · debug-info, Restricted Project