bjope (Bjorn Pettersson)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 26 2016, 7:58 AM (73 w, 3 d)

Recent Activity

Tue, Feb 20

bjope added a comment to D37054: Require address space to be specified when creating functions (2/3).

I've applied your patches to my downstream repo, trying to figure out how much of our own patches for having different address space for functions that can be removed and still getting existing test cases to work when only having your patches.
Lots of new things happens now when there are addrspace on all function definitions. Our old implementation kind of added the addrspace information when fetching function pointers from the symbol table instead of adding addrspace when creating the functions. So I still have a couple of issues to analyse. I think that I at least tracked down one problem with calls to a function given a local function pointer variable (see my inline comment in LLParser.cpp).

There are no testcases included here with ll-files including calls/bitcasts etc with non-zero addrspace, so it is a little bit tricky to understand how far I'm supposed to get by only using your set of patches.
Are you testing this with some other out-of-tree target?

ping!

Tue, Feb 20, 1:15 AM
bjope added inline comments to D43473: [mem2reg] Use range loops (NFCI).
Tue, Feb 20, 12:17 AM

Thu, Feb 8

bjope added a comment to D37054: Require address space to be specified when creating functions (2/3).

I've applied your patches to my downstream repo, trying to figure out how much of our own patches for having different address space for functions that can be removed and still getting existing test cases to work when only having your patches.
Lots of new things happens now when there are addrspace on all function definitions. Our old implementation kind of added the addrspace information when fetching function pointers from the symbol table instead of adding addrspace when creating the functions. So I still have a couple of issues to analyse. I think that I at least tracked down one problem with calls to a function given a local function pointer variable (see my inline comment in LLParser.cpp).

There are no testcases included here with ll-files including calls/bitcasts etc with non-zero addrspace, so it is a little bit tricky to understand how far I'm supposed to get by only using your set of patches.
Are you testing this with some other out-of-tree target?

Thu, Feb 8, 3:25 AM

Jan 10 2018

bjope committed rL322181: Avoid inlining if there is byval arguments with non-alloca address space.
Avoid inlining if there is byval arguments with non-alloca address space
Jan 10 2018, 5:02 AM
bjope closed D41898: Avoid inlining if there is byval arguments with non-alloca address space.
Jan 10 2018, 5:02 AM
bjope updated subscribers of D41898: Avoid inlining if there is byval arguments with non-alloca address space.
Jan 10 2018, 4:22 AM
bjope added a comment to D40455: Teach InlineCost about address spaces.

Dear Bjorn, your added test in byvall.ll expose the problem in inliner causing an assert.

Specifically if I excerpt you test in separate file and run:
$ opt -S -inline -instcombine test.ll
I've got a
opt: ../../lib/Analysis/ValueTracking.cpp:1551: void computeKnownBits(const llvm::Value*, llvm::KnownBits&, unsigned int, const {anonymous}::Query&): Assertion `Q.DL.getTypeSizeInBits(V->getType()->getScalarType()) == BitWidth && "V and Known should have same BitWidth"' failed.

The reason of this is that %p is substituted with

%d = alloca %struct.S1, align 8

which is from addrspace(0) and has a different pointer size than %p.

I'm far from thinking that your patch is causing this issue but probably it is long standing issue in inliner. But it causes the crash in our custom pipeline.
I do not want to revert your patch if you can quickly update the test or simply revert the test itself.

A separate bug against inliner can be filed...

Does it make sense to you?

Ok. I'll take a look at it.

Jan 10 2018, 4:21 AM
bjope created D41898: Avoid inlining if there is byval arguments with non-alloca address space.
Jan 10 2018, 4:19 AM
bjope added a comment to D40455: Teach InlineCost about address spaces.

Dear Bjorn, your added test in byvall.ll expose the problem in inliner causing an assert.

Specifically if I excerpt you test in separate file and run:
$ opt -S -inline -instcombine test.ll
I've got a
opt: ../../lib/Analysis/ValueTracking.cpp:1551: void computeKnownBits(const llvm::Value*, llvm::KnownBits&, unsigned int, const {anonymous}::Query&): Assertion `Q.DL.getTypeSizeInBits(V->getType()->getScalarType()) == BitWidth && "V and Known should have same BitWidth"' failed.

The reason of this is that %p is substituted with

%d = alloca %struct.S1, align 8

which is from addrspace(0) and has a different pointer size than %p.

I'm far from thinking that your patch is causing this issue but probably it is long standing issue in inliner. But it causes the crash in our custom pipeline.
I do not want to revert your patch if you can quickly update the test or simply revert the test itself.

A separate bug against inliner can be filed...

Does it make sense to you?

Jan 10 2018, 12:51 AM

Jan 9 2018

bjope added a comment to D41226: [LiveDebugValues] recognize spilled register that is killed in instruction after the spill.

I agree that this is a solution to a specific case. I would be OK with it going in for now (after addressing the remaining review comments) with a FIXME comment added to provide a more comprehensive solution later, i.e. one that addresses bundles and kills in instructions further down the chain, as Bjorn suggested.

Jan 9 2018, 1:32 PM

Jan 7 2018

bjope added inline comments to D41226: [LiveDebugValues] recognize spilled register that is killed in instruction after the spill.
Jan 7 2018, 12:43 PM

Jan 6 2018

bjope added a comment to D41226: [LiveDebugValues] recognize spilled register that is killed in instruction after the spill.

I do not quite get it why the case when the register is killed in the subsequent instructions is so interesting.
Is this some kind of heuristic just to cover one specific scenario (then I think the summary, and future commit message should mention that, and also add some code comments describing what is going on here).

Jan 6 2018, 6:22 PM

Jan 5 2018

bjope committed rL321907: [DebugInfo] Align comments in debug_loc section.
[DebugInfo] Align comments in debug_loc section
Jan 5 2018, 2:21 PM
bjope closed D41763: [DebugInfo] Align comments in debug_loc section.
Jan 5 2018, 2:21 PM
bjope updated subscribers of D41763: [DebugInfo] Align comments in debug_loc section.
Jan 5 2018, 4:59 AM
bjope added reviewers for D41763: [DebugInfo] Align comments in debug_loc section: JDevlieghere, rnk, aprantl.
Jan 5 2018, 4:55 AM
bjope created D41763: [DebugInfo] Align comments in debug_loc section.
Jan 5 2018, 4:51 AM

Jan 4 2018

bjope added a comment to D1251: Teach InlineCost about address spaces.

I believe this review is superseded by https://reviews.llvm.org/rL321809, so it can probably be abandoned now.

Jan 4 2018, 10:33 AM
bjope committed rL321809: Teach InlineCost about address spaces.
Teach InlineCost about address spaces
Jan 4 2018, 10:25 AM
bjope closed D40455: Teach InlineCost about address spaces.
Jan 4 2018, 10:24 AM

Dec 18 2017

bjope added inline comments to D40945: [ScalarEvolution] Improve high cost heuristic in SCEVExpander..
Dec 18 2017, 10:23 AM
bjope added a comment to D40455: Teach InlineCost about address spaces.

ping

Dec 18 2017, 7:31 AM

Dec 15 2017

bjope added a comment to D37054: Require address space to be specified when creating functions (2/3).

I've applied your patches to my downstream repo, trying to figure out how much of our own patches for having different address space for functions that can be removed and still getting existing test cases to work when only having your patches.
Lots of new things happens now when there are addrspace on all function definitions. Our old implementation kind of added the addrspace information when fetching function pointers from the symbol table instead of adding addrspace when creating the functions. So I still have a couple of issues to analyse. I think that I at least tracked down one problem with calls to a function given a local function pointer variable (see my inline comment in LLParser.cpp).

Dec 15 2017, 10:11 AM

Dec 14 2017

bjope committed rL320700: [ScalarEvolution] Fix base condition in isNormalAddRecPHI..
[ScalarEvolution] Fix base condition in isNormalAddRecPHI.
Dec 14 2017, 6:48 AM
bjope closed D40946: [ScalarEvolution] Fix base condition in isNormalAddRecPHI..
Dec 14 2017, 6:48 AM

Dec 12 2017

bjope added a comment to D40455: Teach InlineCost about address spaces.

Actually the test seems to just cover the ptrtoint case. Need some for the GEP cases as well

Dec 12 2017, 4:48 AM

Dec 7 2017

bjope added inline comments to D37052: Add default address space for functions to the data layout (1/3).
Dec 7 2017, 9:17 AM
bjope added inline comments to D37052: Add default address space for functions to the data layout (1/3).
Dec 7 2017, 3:45 AM

Dec 6 2017

bjope added inline comments to D40648: A few initializations to please Valgrind. NFC.
Dec 6 2017, 7:12 AM

Dec 5 2017

bjope committed rL319775: Add REQUIRES asserts in combine_loads_from_build_pair.ll.
Add REQUIRES asserts in combine_loads_from_build_pair.ll
Dec 5 2017, 7:26 AM
bjope committed rL319771: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads.
[DAGCombine] Handle big endian correctly in CombineConsecutiveLoads
Dec 5 2017, 6:50 AM
bjope closed D40444: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads by committing rL319771: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads.
Dec 5 2017, 6:50 AM

Dec 4 2017

bjope updated the summary of D40455: Teach InlineCost about address spaces.
Dec 4 2017, 1:50 PM
bjope updated the summary of D40455: Teach InlineCost about address spaces.
Dec 4 2017, 1:50 PM
bjope updated the diff for D40455: Teach InlineCost about address spaces.

Added some more tests.

Dec 4 2017, 1:48 PM
bjope updated the diff for D40444: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads.

Added a test case that checks that the build_pair -> combine load transform is done during ISel.

Dec 4 2017, 10:42 AM

Dec 3 2017

bjope added a comment to D35985: Skip live range segment verification for reserved physregs.

I would expect the live ranges of register units that are reserved to be empty and therefore naturally pass verification. Do you know why you end up having non-empty information in those live ranges? We should identify the cause for that and fix it.

Dec 3 2017, 2:57 AM

Nov 30 2017

bjope added inline comments to D40339: Use getStoreSize() in various places instead of BitSize >> 3.
Nov 30 2017, 3:17 PM

Nov 28 2017

bjope added a comment to D40339: Use getStoreSize() in various places instead of BitSize >> 3.

Thanks for review. r319173.

BTW,

What about all the cases using Type* , like 'ByteSize = Ty->getSizeInBits() / 8;'... These should also be fixed, I presume, but I don't see a getStoreSize() method or similar in Type...

Also, with

diff --git a/include/llvm/IR/DebugInfoMetadata.h b/include/llvm/IR/DebugInfoMetadata.h
index c515f6d..a8aa195 100644
--- a/include/llvm/IR/DebugInfoMetadata.h
+++ b/include/llvm/IR/DebugInfoMetadata.h
@@ -594,6 +594,7 @@ public:
   unsigned getLine() const { return Line; }
   uint64_t getSizeInBits() const { return SizeInBits; }
   uint32_t getAlignInBits() const { return AlignInBits; }
+  uint64_t getSizeInBytes() const { return (SizeInBits < 8 ? 1 : SizeInBits >> 3); }
Nov 28 2017, 8:14 AM
bjope accepted D40339: Use getStoreSize() in various places instead of BitSize >> 3.

LGTM

Nov 28 2017, 4:38 AM

Nov 27 2017

bjope added a comment to D40444: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads.

I've done a double check myself for all in-tree uses and all the uses of areNonVolatileConsecutiveLoads should be fine as well.

So modulo a test case this LGTM.

Nov 27 2017, 8:41 AM
bjope added a comment to D40444: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads.

This endianness problem is probably also latent where do load combination, but we should sink this check into areNonVolatileConsecutiveLoads and MatchLoadCombine.

Nov 27 2017, 7:16 AM

Nov 26 2017

bjope added a comment to D37230: Set hasSideEffects=0 for TargetOpcode::BUNDLE.

A BUNDLE instruction is supposed to describe the combined properties of the bundled instructions (at least when it comes to machine operands). Properties like hasSideEffects/mayLoad/mayStore are static, so we can't update those properties for the BUNDLE instruction depending on the properties of the bundled instructions. So maybe it is a good idea to have the defensive approach of letting the BUNDLE instruction have hasSideEffects=1.

Nov 26 2017, 10:59 AM

Nov 25 2017

bjope added a reviewer for D40455: Teach InlineCost about address spaces: haicheng.
Nov 25 2017, 10:20 AM
bjope added reviewers for D40455: Teach InlineCost about address spaces: chandlerc, arsenm.
Nov 25 2017, 10:20 AM
bjope created D40455: Teach InlineCost about address spaces.
Nov 25 2017, 6:17 AM
bjope added inline comments to D40064: Do not use default arguments of DataLayout::getPointer*. NFC.
Nov 25 2017, 1:15 AM

Nov 24 2017

bjope added reviewers for D40444: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads: niravd, hfinkel.
Nov 24 2017, 9:29 AM
bjope created D40444: [DAGCombine] Handle big endian correctly in CombineConsecutiveLoads.
Nov 24 2017, 9:21 AM
bjope added a comment to D40339: Use getStoreSize() in various places instead of BitSize >> 3.

I have no further comments. And it looks good from our out-of-tree-targets perspective.
In many situations I think this is NFC. For example the consecutive load/store optimizations have other checks verifying that the involved types are byte sized, so the end result will be the same.
But I haven't really reviewed the MIPS/Hexagon specific parts, and there are no test cases showing that something is more correct (I'm not sure all changes are NFC).

Nov 24 2017, 7:08 AM

Nov 23 2017

bjope added inline comments to D40339: Use getStoreSize() in various places instead of BitSize >> 3.
Nov 23 2017, 10:15 AM

Nov 6 2017

bjope committed rL317513: [MIRPrinter] Use %subreg.xxx syntax for subregister index operands.
[MIRPrinter] Use %subreg.xxx syntax for subregister index operands
Nov 6 2017, 1:46 PM
bjope closed D39696: [MIRPrinter] Use %subreg.xxx syntax for subregister index operands by committing rL317513: [MIRPrinter] Use %subreg.xxx syntax for subregister index operands.
Nov 6 2017, 1:46 PM
bjope created D39696: [MIRPrinter] Use %subreg.xxx syntax for subregister index operands.
Nov 6 2017, 1:30 PM

Nov 2 2017

bjope committed rL317198: [SimplifyCFG] Discard speculated dbg intrinsics.
[SimplifyCFG] Discard speculated dbg intrinsics
Nov 2 2017, 4:56 AM
bjope closed D39494: [SimplifyCFG] Discard speculated dbg intrinsics by committing rL317198: [SimplifyCFG] Discard speculated dbg intrinsics.
Nov 2 2017, 4:55 AM

Nov 1 2017

bjope created D39494: [SimplifyCFG] Discard speculated dbg intrinsics.
Nov 1 2017, 8:52 AM

Oct 26 2017

bjope committed rL316665: [LSV] Avoid adding vectors of pointers as candidates.
[LSV] Avoid adding vectors of pointers as candidates
Oct 26 2017, 6:59 AM
bjope closed D39296: [LSV] Avoid adding vectors of pointers as candidates by committing rL316665: [LSV] Avoid adding vectors of pointers as candidates.
Oct 26 2017, 6:59 AM
bjope committed rL316663: [LSV] Skip all non-byte sizes, not only less than eight bits.
[LSV] Skip all non-byte sizes, not only less than eight bits
Oct 26 2017, 6:43 AM
bjope closed D39295: [LSV] Skip all non-byte sizes, not only less than eight bits by committing rL316663: [LSV] Skip all non-byte sizes, not only less than eight bits.
Oct 26 2017, 6:43 AM
bjope added inline comments to D39295: [LSV] Skip all non-byte sizes, not only less than eight bits.
Oct 26 2017, 6:42 AM
bjope added a comment to D34272: [Tooling] A new framework for executing clang frontend actions..

I get errors when compiling due to this commit:

Oct 26 2017, 5:50 AM

Oct 25 2017

bjope added a reviewer for D39295: [LSV] Skip all non-byte sizes, not only less than eight bits: arsenm.
Oct 25 2017, 9:42 AM
bjope added a reviewer for D39296: [LSV] Avoid adding vectors of pointers as candidates: arsenm.
Oct 25 2017, 9:42 AM
bjope created D39296: [LSV] Avoid adding vectors of pointers as candidates.
Oct 25 2017, 9:26 AM
bjope created D39295: [LSV] Skip all non-byte sizes, not only less than eight bits.
Oct 25 2017, 9:25 AM

Oct 24 2017

bjope committed rL316430: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
[ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP
Oct 24 2017, 5:08 AM
bjope closed D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP by committing rL316430: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
Oct 24 2017, 5:08 AM
bjope committed rL316429: [LangRef] Update description of Constant Expressions.
[LangRef] Update description of Constant Expressions
Oct 24 2017, 5:00 AM
bjope closed D39165: [LangRef] Update description of Constant Expression by committing rL316429: [LangRef] Update description of Constant Expressions.
Oct 24 2017, 5:00 AM
bjope added inline comments to D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
Oct 24 2017, 12:57 AM

Oct 22 2017

bjope added a comment to D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.

Needs a FIXME or something explaining why exactly you're returning early; in theory, we should be able to fold vector ptrtoint operations the same way we fold integer ptrtoint operations, given the right logic.

Oct 22 2017, 3:58 AM
bjope created D39165: [LangRef] Update description of Constant Expression.
Oct 22 2017, 3:52 AM
bjope updated the diff for D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.

Add a FIXME as requested by Eli.

Oct 22 2017, 3:20 AM

Oct 18 2017

bjope requested review of D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
Oct 18 2017, 11:45 PM
bjope added a comment to D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.

The constant expression ptrtoint is supposed to have the same rules as the instruction ptrtoint. Looks like we just forgot to update LangRef when pointer vectors were added.

Oct 18 2017, 8:40 AM

Oct 17 2017

bjope added inline comments to D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
Oct 17 2017, 2:53 PM

Oct 16 2017

bjope added a comment to D1251: Teach InlineCost about address spaces.

If nobody remembers why this very old patch never was landed, then I guess I'll make a new attempt as it would avoid the asserts that I've seen.
And it is also one step towards getting rid of the default arguments in DataLayout::getPointerSizeInBits(unsigned AS = 0) , marked as a FIXME in DataLayout.h.

Oct 16 2017, 2:19 PM
bjope added inline comments to D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
Oct 16 2017, 1:59 PM
bjope added a comment to D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.

Ping!

Oct 16 2017, 6:00 AM

Oct 12 2017

bjope added a comment to D38830: [DWARF] Fix bad comparator in sortGlobalExprs..

Eli, I think this looks good.

Oct 12 2017, 2:52 AM

Oct 11 2017

bjope added a comment to D1251: Teach InlineCost about address spaces.

Does anyone remember why this has been "abandoned" (still not finished after four years being in need for review)?

Oct 11 2017, 1:40 AM

Oct 9 2017

bjope added a reviewer for D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP: davide.
Oct 9 2017, 6:27 AM

Oct 4 2017

bjope added inline comments to D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
Oct 4 2017, 9:01 AM
bjope created D38546: [ConstantFolding] Avoid assert when folding ptrtoint of vectorized GEP.
Oct 4 2017, 8:54 AM

Oct 3 2017

bjope committed rL314781: [DebugInfo] Handle endianness when moving debug info for split integer values….
[DebugInfo] Handle endianness when moving debug info for split integer values…
Oct 3 2017, 4:04 AM
bjope added a comment to D38172: [Debug info] Handle endianness when moving debug info for split integer values.

Hi,

The test doesn't pass on a llc built without the target "AMDGPU" enabled. To reproduce:

  1. Add -DLLVM_TARGETS_TO_BUILD="X86;PowerPC" to the cmake command.
  2. re-run cmake and build llc. The test doesn't pass on that llc.

    Notice that if we define LLVM_TARGETS_TO_BUILD as "X86;PowerPC;AMDGPU", the test passes again.

    I think this is related to [1], but I don't understand why "isel" defined in AMDGPU gets picked up by a PPC32 through "-stop-after=isel".

    [1] https://github.com/llvm-mirror/llvm/blob/7287fcb5d580e8ac0b860ddd4da03dc395795fa0/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp#L237
Oct 3 2017, 2:27 AM

Oct 2 2017

bjope committed rL314667: [X86][SSE] Fix -Wsign-compare problems introduced in r314658.
[X86][SSE] Fix -Wsign-compare problems introduced in r314658
Oct 2 2017, 5:48 AM
bjope committed rL314666: [Debug info] Handle endianness when moving debug info for split integer values.
[Debug info] Handle endianness when moving debug info for split integer values
Oct 2 2017, 5:48 AM
bjope closed D38172: [Debug info] Handle endianness when moving debug info for split integer values by committing rL314666: [Debug info] Handle endianness when moving debug info for split integer values.
Oct 2 2017, 5:48 AM

Sep 28 2017

bjope committed rL314414: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.
[DebugInfo] Do not extend range for physreg in LiveDebugVariables
Sep 28 2017, 6:14 AM
bjope closed D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables by committing rL314414: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.
Sep 28 2017, 6:14 AM

Sep 27 2017

bjope added a comment to D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.

Okay, let's do this: Split the CHECKDBG test into a separate .test file that has the REQUIRES: asserts and re-uses the same input. This way we get some testing in all configurations and full testing in asserts builds.

Sep 27 2017, 9:45 AM
bjope added a comment to D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.

live-debug-variables happens so late in the pipeline that in practice checking for dwarfdump output hasn't caused much churn in the testsuite. I would prefer a -filetype=obj -start-before=greedy | llvm-dwarfdump- test over one that checks DEBUG output (which will only be tested by bots that build with assertions enabled).

Sep 27 2017, 2:18 AM
bjope updated the diff for D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.

Narrowed the test case even further by stopping already after virtregrewriter.

Sep 27 2017, 1:39 AM

Sep 26 2017

bjope added inline comments to D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.
Sep 26 2017, 3:30 PM
bjope updated the diff for D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.

Added REQUIRES asserts to the test case.
Added a second RUN line to also verify the MIR.

Sep 26 2017, 3:30 PM
bjope added inline comments to D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables.
Sep 26 2017, 3:30 PM

Sep 25 2017

bjope added reviewers for D38140: [DebugInfo] Do not extend range for physreg in LiveDebugVariables: aprantl, rnk, echristo.
Sep 25 2017, 5:03 AM