Page MenuHomePhabricator

pvellien (praveen velliengiri)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 24 2020, 11:41 PM (78 w, 3 d)

Recent Activity

Jan 18 2022

pvellien added a comment to D116216: Prevent adding module flag - amdgpu_hostcall multiple times..

LGTM. Thanks.

Jan 18 2022, 8:38 AM · Restricted Project
pvellien updated subscribers of rG4e1c2077262e: [SimplifyCFG] Fix assertion failure when reusing table switch comparison.

@nikic thanks.

Jan 18 2022, 12:32 AM
pvellien updated the diff for D116216: Prevent adding module flag - amdgpu_hostcall multiple times..

Removed amdgpu-asan-noprintf.cu and added amdgpu-asan-printf.cu testcase.

Jan 18 2022, 12:23 AM · Restricted Project
pvellien added a comment to D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.

@nikic thanks a lot for accepting. Could you please commit this patch on my behalf? I don't have a commit access to llvm. thanks

Sure, can you let me know what Name <email> to use for the commit?

Jan 18 2022, 12:17 AM · Restricted Project
pvellien added a comment to D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.

@nikic thanks a lot for accepting. Could you please commit this patch on my behalf? I don't have a commit access to llvm. thanks

Jan 18 2022, 12:13 AM · Restricted Project

Jan 17 2022

pvellien updated the diff for D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.

Address review comments. thanks

Jan 17 2022, 11:19 PM · Restricted Project
pvellien updated the diff for D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.

@nikic Thanks a lot for the detailed description to reduce the test-case, it greatly helped me.
Updated the patch with test-cases.

Jan 17 2022, 5:54 AM · Restricted Project
pvellien added a comment to D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.

Hi @nikic
https://gist.github.com/pvellien/e4f519d17b25adf10fdd5978ee0b1de9
After trying a lot, this is minimal IR i can able to find. It is reduction from 84k lines to 617 lines.
Please compile with Opt with O3.

Jan 17 2022, 1:43 AM · Restricted Project

Jan 13 2022

pvellien added a comment to D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.

hi @nikic I tired both bugpoint and llvm-reduce both of them, failed to reduce the issue. It would be very much helpful to know how to proceed further. I'm new to the llvm middle-end, I could really use some help here.
If it will be helpful I will share the complete IR module which fails at assert.

Jan 13 2022, 10:55 AM · Restricted Project
pvellien added a comment to D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.

@nikic generating a small test-case is really hard, since it was triggering due to small peephole under very specific constraints. what would you advise here?

Jan 13 2022, 5:28 AM · Restricted Project

Jan 12 2022

pvellien requested review of D117184: Fix failing assertion in SimplifyCFG.cpp as icmp gep fold to constant expression.
Jan 12 2022, 10:46 PM · Restricted Project

Jan 11 2022

pvellien added a comment to rG5afbfe33e7d6: [ConstantFold] Make icmp of gep fold offset based.

thanks for your reply @nikic
I'm trying to reduce the test-case to a minimal one. It may take some time. I will share as soon as I have it.
If I have to relax the assert, can I check whether the constant expr is a compare instead of true and false const?
Or If I have to use target dependent constant folding, how to do that?

Jan 11 2022, 9:46 AM
pvellien added a comment to rG5afbfe33e7d6: [ConstantFold] Make icmp of gep fold offset based.

thanks for your reply @nikic
I'm trying to reduce the test-case to a minimal one. It may take some time. I will share as soon as I have it.
If I have to relax the assert, can I check whether the constant expr is a compare instead of true and false const?
Or If I have to use target dependent constant folding, how to do that?

Jan 11 2022, 7:03 AM
pvellien added a comment to rG5afbfe33e7d6: [ConstantFold] Make icmp of gep fold offset based.

Hi @nikic This patch triggers an assertion in simplifycfg.cpp pass.
https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Transforms/Utils/SimplifyCFG.cpp$5800 this assertion is failing because that CaseConst returned is neither a true or false constant but a compare constant expression.
In my case, the instruction that triggers this assertion is

Jan 11 2022, 4:58 AM

Jan 9 2022

pvellien updated the diff for D116216: Prevent adding module flag - amdgpu_hostcall multiple times..

@lebedev.ri updated with test-cases.

Jan 9 2022, 11:34 PM · Restricted Project

Dec 24 2021

pvellien added a comment to D116216: Prevent adding module flag - amdgpu_hostcall multiple times..

@yaxunl It would be very much helpful to know how to write test coverage for this particular patch? thanks

Dec 24 2021, 8:58 AM · Restricted Project

Dec 23 2021

pvellien added a comment to D116216: Prevent adding module flag - amdgpu_hostcall multiple times..

The testcases related to this patch are already added via this patch https://reviews.llvm.org/D112820.

Dec 23 2021, 8:52 AM · Restricted Project
pvellien added reviewers for D116216: Prevent adding module flag - amdgpu_hostcall multiple times.: yaxunl, b-sumner.
Dec 23 2021, 3:03 AM · Restricted Project
pvellien requested review of D116216: Prevent adding module flag - amdgpu_hostcall multiple times..
Dec 23 2021, 3:03 AM · Restricted Project

Nov 10 2021

pvellien added a comment to D112820: Emit hidden hostcall argument for sanitized kernels..

Thanks for accepting. I don't have commit access to llvm trunk yet. Could you please commit this patch? thanks.

Nov 10 2021, 8:31 AM · Restricted Project, Restricted Project
pvellien updated the diff for D112820: Emit hidden hostcall argument for sanitized kernels..

Added a check in amdgpu-asan.cu file

Nov 10 2021, 12:53 AM · Restricted Project, Restricted Project

Nov 2 2021

pvellien updated the diff for D112820: Emit hidden hostcall argument for sanitized kernels..

Addressed comments.

Nov 2 2021, 10:51 AM · Restricted Project, Restricted Project
pvellien updated the diff for D112820: Emit hidden hostcall argument for sanitized kernels..

rebased

Nov 2 2021, 6:53 AM · Restricted Project, Restricted Project
pvellien updated the diff for D112820: Emit hidden hostcall argument for sanitized kernels..

Addressed review comments.

Nov 2 2021, 6:50 AM · Restricted Project, Restricted Project

Oct 29 2021

pvellien requested review of D112820: Emit hidden hostcall argument for sanitized kernels..
Oct 29 2021, 7:54 AM · Restricted Project, Restricted Project

Sep 27 2021

pvellien added a comment to D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external..

I can help commit this patch. Can you please rebase your work with the latest main branch and upload the patch again, @pvellien ? Thanks

Sep 27 2021, 9:28 AM · Restricted Project
pvellien updated the diff for D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external..

Rebase on top.

Sep 27 2021, 9:27 AM · Restricted Project
pvellien added inline comments to D110468: [AMDGPU] Do not internalize ASan device library functions..
Sep 27 2021, 1:00 AM · Restricted Project

Sep 22 2021

pvellien added a comment to D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external..

Hi, The failures in the debian builds are not related to this patch changes. I don't have a commit access to llvm trunk, could any of you please land this patch on my behalf. Thanks a lot for your time.

Sep 22 2021, 8:21 AM · Restricted Project

Sep 21 2021

pvellien added a comment to D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external..

@foad thanks.

Sep 21 2021, 3:21 AM · Restricted Project
pvellien retitled D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external. from [AMDGPU] - change ASAN init/fini kernels linkage to external. to [AMDGPU] Change ASAN init/fini kernels linkage to external..
Sep 21 2021, 3:20 AM · Restricted Project
pvellien updated the diff for D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external..

Added test.

Sep 21 2021, 2:34 AM · Restricted Project

Sep 20 2021

pvellien added a comment to D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external..

Hi @yaxunl It will helpful to know what kind of test is expected to test the linkage type of generated init/fini kernels. The related test-cases for init/fini kernels are added in this patch - https://reviews.llvm.org/D105682

Sep 20 2021, 10:52 AM · Restricted Project
pvellien requested review of D110054: [AMDGPU] Change ASAN init/fini kernels linkage to external..
Sep 20 2021, 2:00 AM · Restricted Project

Feb 25 2021

pvellien updated subscribers of D95232: Symbolizer - Teach symbolizer to work directly on object file..
Feb 25 2021, 12:35 AM · Restricted Project

Feb 22 2021

pvellien added a comment to D95232: Symbolizer - Teach symbolizer to work directly on object file..

(CC @jhenderson @rnk )

The issues with the const std::string & parameter symbolize* API:

  • The LLVMSymbolizer instance needs to construct ObjectFile, which may be a duplicate if the application has an existing ObjectFile instance.
  • The file reading error is buried in the API. The application cannot easily tell there is an IO error or incorrect address error and may keep symbolizing more addresses after an IO error. In addition, the user may want to handle the IO error themselves.
  • It does magic things (opening an auxiliary file) which may be of security concern for some usage: it constructs a .dsym path for a Mach-O file; it constructs a path from build ID for an ELF file; it constructs a .gnu_debuglink path if the section is present in an ELF file; it constructs a PDB path for a PE file. Personally I'd like the feature to open auxiliary files to be specified explicitly by the user on ELF.

These ^ are the reasons you're suggesting it would be good to refactor the existing code to take an ObjectFile instead of a std::string file name?

Yes

The const ObjectFile & symbolize* does not have the ability to open magic auxiliary files. This is not clear from the API.

This ^ is a limitation of these newly proposed ObjectFile APIs? So they won't be entirely compatible with the existing string-based APIs? So it would be difficult/not possible to refactor the existing code to use these APIs?

I think we can pass the file with debuginfo as an additional parameter as suggested by @MaskRay regarding refactoring all the use-cases within llvm to new APIs I'm not sure how complicated it would be in tools such as symbolizer, objdump etc

Feb 22 2021, 9:24 PM · Restricted Project

Feb 18 2021

pvellien added a comment to D95232: Symbolizer - Teach symbolizer to work directly on object file..

I'm not sure if this is what @dblaikie was referring to, but it seems like a good way to test new public-facing APIs would be to write gtest unit tests. I don't think there's any precedent for this for the symbolizer API, so it may not be particularly easy. Refactoring llvm-symbolizer to make use of the new API is probably a good idea, and it may help with the test coverage, but there is a risk that a future refactor beyond that will cause llvm-symbolizer to no longer use the API, and therefore the test coverage is lost.

Feb 18 2021, 8:58 AM · Restricted Project
pvellien added a comment to D95232: Symbolizer - Teach symbolizer to work directly on object file..

(CC @jhenderson @rnk )

The issues with the const std::string & parameter symbolize* API:

  • The LLVMSymbolizer instance needs to construct ObjectFile, which may be a duplicate if the application has an existing ObjectFile instance.
  • The file reading error is buried in the API. The application cannot easily tell there is an IO error or incorrect address error and may keep symbolizing more addresses after an IO error. In addition, the user may want to handle the IO error themselves.
  • It does magic things (opening an auxiliary file) which may be of security concern for some usage: it constructs a .dsym path for a Mach-O file; it constructs a path from build ID for an ELF file; it constructs a .gnu_debuglink path if the section is present in an ELF file; it constructs a PDB path for a PE file. Personally I'd like the feature to open auxiliary files to be specified explicitly by the user on ELF.

The const ObjectFile & symbolize* does not have the ability to open magic auxiliary files. This is not clear from the API.

If we want to keep just one set of symbolize* overloads and go for const ObjectFile &, we perhaps need to store ObjectFile handles instead of StringRef handles for LLVMSymbolizer's internal maps, and make Expected<SymbolizableModule *> LLVMSymbolizer::getOrCreateModuleInfo(const std::string &ModuleName) public.

I want to check with folks whether whether some refactoring is needed in this area.


This can be useful for clients who already have a in-memory object files to symbolize for.

I'd hear more about the use case. For in-memory object files, how do you handle .gnu_debuglink and the build ID stuff. It is possible that you don't want to handle this.

Feb 18 2021, 8:51 AM · Restricted Project
pvellien added a comment to D95232: Symbolizer - Teach symbolizer to work directly on object file..

From my reads of the history of this component, I think DebugInfo/Symbolize was extracted from llvm-symbolizer because sanitizer runtime needs it. Later other LLVM internal tools (sanitizer runtime, sancov, sanstats, llvm-xray, etc) use the API as well.
I don't find usage from open-source projects. There could be, but I speculate that if do some refactoring the friction will be small.

Currently the std::string overloads are mainly used. It seems to me that we don't need to std::string overloads. They can likely all switch to the const ObjectFile & overloads.
I will take a stab at cleaning up the call sites.

@pvellien @scott.linder The concern with not testing the new API is that they are otherwise unused (I guess that you may have downstream projects which may adopt them soon) and may be deleted by other contributors as dead code.
If you have some specific use cases, contributing unittests would probably be a good idea.
I'll try refactoring the API, if const std::string& is replaced with const ObjectFile & overloads, then the API will be used by in-tree code and will be less likely deleted as dead code.

Feb 18 2021, 8:38 AM · Restricted Project

Feb 16 2021

pvellien added a comment to D95232: Symbolizer - Teach symbolizer to work directly on object file..

Generally there should be test coverage - otherwise these APIs are likely to be cleaned up as unused, even if the chance of actual bugs in them is small (since they're just simple wrappers).

Perhaps some unit testing would be suitable?

Hi dave, thanks for your comments. I was worried about this thing. But there are no existing tools in llvm that work on object file directly expect there is a usage for one existing API - symbolizeCode.

Sorry, I'm not quite following this last sentence, could you rephrase it?

The first part (" But there are no existing tools in llvm that work on object file directly ") doesn't seem quite right, so far as I'm reading it - llvm-symbolizer, the command line tool, can operate on object files and many of the llvm-symbolizer tests operate on object files. (which also makes me wonder about this change in general - llvm-symbolizer can already work on object files, so why are new APIs required, given the functionality already exists/is used/tested?)

I don't know how it is reliable to add unit tests, I would highly appreciate your thoughts on this. thanks

Feb 16 2021, 10:52 AM · Restricted Project
pvellien added a comment to D95232: Symbolizer - Teach symbolizer to work directly on object file..

Generally there should be test coverage - otherwise these APIs are likely to be cleaned up as unused, even if the chance of actual bugs in them is small (since they're just simple wrappers).

Perhaps some unit testing would be suitable?

Feb 16 2021, 9:07 AM · Restricted Project

Feb 7 2021

pvellien added a comment to D95232: Symbolizer - Teach symbolizer to work directly on object file..

ping

Feb 7 2021, 10:52 PM · Restricted Project

Feb 3 2021

pvellien updated the diff for D95232: Symbolizer - Teach symbolizer to work directly on object file..

Added FIXME

Feb 3 2021, 10:54 PM · Restricted Project

Jan 27 2021

pvellien updated the diff for D95232: Symbolizer - Teach symbolizer to work directly on object file..

Refactor code

Jan 27 2021, 10:43 AM · Restricted Project

Jan 25 2021

pvellien added reviewers for D95232: Symbolizer - Teach symbolizer to work directly on object file.: scott.linder, tony-tye.
Jan 25 2021, 9:25 AM · Restricted Project

Jan 22 2021

pvellien requested review of D95232: Symbolizer - Teach symbolizer to work directly on object file..
Jan 22 2021, 6:45 AM · Restricted Project

Dec 24 2020

pvellien abandoned D92115: AMDGPU - Add diagnostic for compiling modules with AMD HSA OS type and GFX 6 arch .

This change is wrong, the different patch is landed in llvm to handle global address space access in gfx60x for HSA Os. So closing it.

Dec 24 2020, 2:24 AM · Restricted Project, Restricted Project
pvellien added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 24 2020, 12:21 AM · Restricted Project
pvellien updated the diff for D92483: AMDGPU - Use MUBUF instructions for global address space access.

Fixed a small typo in comments. Nothing else is changed from prior diff.

Dec 24 2020, 12:18 AM · Restricted Project
pvellien updated the diff for D92483: AMDGPU - Use MUBUF instructions for global address space access.

Included the suggestions given by tony.

Dec 24 2020, 12:07 AM · Restricted Project

Dec 23 2020

pvellien added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 23 2020, 10:45 PM · Restricted Project
pvellien added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.

If this is fine, place land. I don't have commit access yet.

Dec 23 2020, 11:19 AM · Restricted Project
pvellien added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 23 2020, 10:22 AM · Restricted Project
pvellien updated the diff for D92483: AMDGPU - Use MUBUF instructions for global address space access.

added a blank line in amdgpuusage.rst file.

Dec 23 2020, 9:42 AM · Restricted Project
pvellien updated the diff for D92483: AMDGPU - Use MUBUF instructions for global address space access.

Updated the AMDGPUUsage doc. Also I don't have commit access, please commit this patch on my behalf.
Thanks a lot your time.

Dec 23 2020, 7:07 AM · Restricted Project

Dec 22 2020

pvellien updated the diff for D92483: AMDGPU - Use MUBUF instructions for global address space access.

Updated the code according to scott's suggestion. And I tried to compile sample program with all possible combinations of processors and ABIs( amdhsa, mesa3d, amdpal), it seems like the backend supports all the ABIs for all processors. So Instead of adding a separate column, (since values of processors will be the same) I added a text pharse above the processor table which documents. Is it right thing to do so?

Dec 22 2020, 12:42 PM · Restricted Project

Dec 21 2020

pvellien added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.

But I still do not think this is a right thing to do to accept amdhsa on SI, even if you turn off flat instructions. SI does not support HSA.

As far as I the support of HSA in SI processors. Tony suggested that gfx60x processors should support AMDHSA OS because it is capable of supporting OpenCL 1.2 which not need generic pointers. This topic was discussed in amdgcn weekly and using MUBUF instructions for global address space in gfx60x is decided. The same is also documented in AMDGPU User Guide earlier.

But it is not. In fact AMDGPUUsage.rst says that SI does NOT support amdhsa and it is only supported starting from gfx700:

**GCN GFX6 (Southern Islands (SI))** [AMD-GCN-GFX6]_
-----------------------------------------------------------------------------------------------------------------------
``gfx600``  - ``tahiti``    ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
                                                                   support
                                                                   generic
                                                                   address
                                                                   space
``gfx601``  - ``pitcairn``  ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
            - ``verde``                                            support
                                                                   generic
                                                                   address
                                                                   space
``gfx602``  - ``hainan``    ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
            - ``oland``                                            support
                                                                   generic
                                                                   address
                                                                   space

It is only amdpal listed here.

What was discussed is that we can use buffer instructions for global on SI even if amdhsa is not supported.

At one point, while you were on vacation, the consensus was "support amdhsa OS with gfx6, and fail to compile in the presence of e.g. generic pointers".

Has there been more discussion since then?

Dec 21 2020, 9:28 AM · Restricted Project

Dec 18 2020

pvellien added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.

if mcpu is specified, set Gen to the correct generation that cpu belongs to, by initializing it with SOUTHERN_ISLANDS.
if mcpu is not specified (in this case GPU = "generic-hsa"), set Gen to the SEA_LANDS to be in parity with previous code> Comments inline

You are forcing SI for any SPECIFIED GPU instead:

!GPU.contains("generic") ? SOUTHERN_ISLANDS

Read this code line: if CPU is NOT generic, force it to SI.

Ugh. I see what you mean now, it took debugger to understand. The real value of Gen is assigned below in the GCNSubtarget::initializeSubtargetDependencies().

But I still do not think this is a right thing to do to accept amdhsa on SI, even if you turn off flat instructions. SI does not support HSA.

Dec 18 2020, 8:49 AM · Restricted Project

Dec 16 2020

pvellien added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.

Comments inline

Dec 16 2020, 8:54 AM · Restricted Project

Dec 11 2020

pvellien added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 11 2020, 10:08 AM · Restricted Project

Dec 10 2020

pvellien added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 10 2020, 11:24 AM · Restricted Project

Dec 8 2020

pvellien added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 8 2020, 10:53 AM · Restricted Project

Dec 7 2020

pvellien updated the diff for D92483: AMDGPU - Use MUBUF instructions for global address space access.

Removed error reporting based on string comparison. Updated the memory legalizer tests to include amdhsa for gfx60x

Dec 7 2020, 5:46 AM · Restricted Project

Dec 2 2020

pvellien added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.

fixed a typo in comments.

Dec 2 2020, 9:10 PM · Restricted Project
pvellien added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.

Comments inline

Dec 2 2020, 9:01 PM · Restricted Project
pvellien requested review of D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 2 2020, 7:07 AM · Restricted Project

Nov 27 2020

pvellien added inline comments to D92115: AMDGPU - Add diagnostic for compiling modules with AMD HSA OS type and GFX 6 arch .
Nov 27 2020, 11:50 AM · Restricted Project, Restricted Project
pvellien added inline comments to D92115: AMDGPU - Add diagnostic for compiling modules with AMD HSA OS type and GFX 6 arch .
Nov 27 2020, 10:40 AM · Restricted Project, Restricted Project
pvellien updated the diff for D92115: AMDGPU - Add diagnostic for compiling modules with AMD HSA OS type and GFX 6 arch .

Updated with stanislav comments

Nov 27 2020, 4:06 AM · Restricted Project, Restricted Project

Nov 25 2020

pvellien requested review of D92115: AMDGPU - Add diagnostic for compiling modules with AMD HSA OS type and GFX 6 arch .
Nov 25 2020, 10:17 AM · Restricted Project, Restricted Project