Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (337 w, 11 h)

Recent Activity

Today

dblaikie added inline comments to D59664: [llvm] Non-functional change: declared a couple of local variables as const..
Mon, Mar 25, 7:07 PM · Restricted Project
dblaikie added a comment to D59008: [AMDGPU] Switch default dwarf version to 5.

LGTM

Do we know the state of split DWARF and DWARF compression for DWARF 5 (compared to DWARF 2)?

State of them in what sense? Compression is pretty orthogonal to any DWARF version - it's more about the container (ELF, etc) you use. Split DWARF is non-standardly supported in pre-v5, and I think it's functioning in the standards conformant v5 mode too.

I had heard mention that at least split DWARF may not be fully supported yet for DWARF 5 in LLVM. But maybe that is old news:-)

Mon, Mar 25, 5:04 PM · Restricted Project
dblaikie added a comment to D59008: [AMDGPU] Switch default dwarf version to 5.

LGTM

Do we know the state of split DWARF and DWARF compression for DWARF 5 (compared to DWARF 2)?

Mon, Mar 25, 12:00 PM · Restricted Project

Fri, Mar 22

dblaikie accepted D59732: [DWARF] Delete one unused DWARFContext::create overload and associated ctor of DWARFObjInMemory.

Assuming you've checked this is unused in other projects like lldb, lld, etc, sounds good to me - thanks!

Fri, Mar 22, 7:22 PM · Restricted Project
dblaikie accepted D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
In D58848#1440161, @avl wrote:

Please move this down at least far enough that it doesn't involve separately
computing the file name and opening the file. Where it's a "well, if you didn't
specify a section index, we'll use the first section that covers this address".

It looks like this version of patch does exactly this.
It does not open file separately and does not parse file name.
The only thing which is additionally done is for already opened file :
"well, if you didn't specify a section index, we'll use the first section that covers this address" :

uint64_t SymbolizableObjectFile::getModuleSectionIndexForAddress(

  uint64_t Address) const {
 
for (SectionRef Sec : Module->sections()) {
  if (!Sec.isText() || Sec.isVirtual())
    continue;
 
  if (Address >= Sec.getAddress() &&
      Address <= Sec.getAddress() + Sec.getSize()) {
    return Sec.getIndex();
  }
}
 
return object::SectionedAddress::UndefSection;

}

I think it looks very close to above description.

Fri, Mar 22, 2:52 PM · Restricted Project
dblaikie added inline comments to D59515: [llvm] Prevent duplicate files in debug line header in dwarf 5..
Fri, Mar 22, 2:23 PM · debug-info, Restricted Project
dblaikie added a comment to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
In D58848#1430203, @avl wrote:

In case Undef specified then there could be several sections with matching address ranges.
It seems to me that DWARFDebugLineTable improper place to make such a decision - which of them should be taken as a result.
This in fact high level decision, not low level.

Fri, Mar 22, 11:53 AM · Restricted Project
dblaikie added inline comments to D59515: [llvm] Prevent duplicate files in debug line header in dwarf 5..
Fri, Mar 22, 11:05 AM · debug-info, Restricted Project

Thu, Mar 21

dblaikie added a comment to D59673: [Driver] Allow setting the DWO name DWARF attribute separately.

Pleasue include mention of the bug (PR40276) in the commit message & clarify that while this is useful for some remote compilation models, it's not strictly necessary/the only way to do it (a remote compilation model that keeps relative paths and uses compilation-dir instead can work without this attribute)

Thu, Mar 21, 3:28 PM · Restricted Project

Wed, Mar 20

dblaikie added a comment to D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used.

Does this do the right thing for a 32-bit target? 64-bit is common, but not universal.

Re. the underlying DWARF issues:
I can't remember whether the DWARF committee has ever considered specifying a special value for "address does not exist" but I doubt it, because the DWARF spec states "If an entity has no associated machine code, none of these [address-related] attributes are specified." (DWARF v5 section 2.17, p.51.) So, the current position of the DWARF spec is that the linker is supposed to rewrite the DWARF, I guess.

Wed, Mar 20, 1:20 PM · lld, Restricted Project
dblaikie updated subscribers of D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used.

This seems like it might be worth a broader discussion about these sort of cases - probably llvm-dev, maybe with some DWARF folks (though the usual LLVM debug info cabal (myself, @aprantl, @probinson, @JDevlieghere, and @echristo) is probably sufficient to get a rough idea of what might be a good general approach).

Wed, Mar 20, 10:37 AM · lld, Restricted Project

Tue, Mar 19

dblaikie added a comment to D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used.

(also the title of this patch makes it sound like this is a patch to llvm-symbolizer (which I Suspect it should be - if gold/binutils ld produce similar output to lld's current (pre-patch) output, then llvm-symbolizer should be adjusted to cope with that, even if lld is changed))

Tue, Mar 19, 11:18 AM · lld, Restricted Project
dblaikie added a comment to D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used.

What do other linkers (gold and gnu binutils, for instance) do in this case?

Tue, Mar 19, 11:18 AM · lld, Restricted Project

Fri, Mar 15

dblaikie added a comment to D59417: [GVN] Add default debug location when constructing PHI nodes.
In D59417#1431153, @vsk wrote:

I believe this is a patch to fix PR37964?

Hrm - if debugify diagnoses all instances of instructions without locations as buggy, that sounds very noisy to me. How does it handle all the other cases of merged locations that end up with no location? (if my recollection is accurate and the code is still doing this - merged locations that aren't the same and that aren't calls get no location, not a zero location)

It looks like DILocation::getMergedLocation() always returns a zero location, and that Instruction::applyMergedLocation() no longer restricts itself to calls. This changed with D45396+D45397: we wanted mem2reg to set merged locations on phis to help users narrow down the scope of a crashing instruction.

Fri, Mar 15, 11:40 AM · Restricted Project
dblaikie added a comment to D59417: [GVN] Add default debug location when constructing PHI nodes.

I believe this is a patch to fix PR37964?

Fri, Mar 15, 10:58 AM · Restricted Project
dblaikie added a comment to D59417: [GVN] Add default debug location when constructing PHI nodes.

Does this need to be line zero, or could it be no location (the absence of a dbgloc)?

Fri, Mar 15, 10:02 AM · Restricted Project

Thu, Mar 14

dblaikie accepted D58952: [llvm] Skip over empty line table entries..

Thanks a bunch - looks good to me.

Thu, Mar 14, 5:40 PM · debug-info, Restricted Project
dblaikie added a comment to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
In D58848#1430071, @avl wrote:

I moved handling Undef case from Symbolizer into SymbolizableObjectFile. Please check whether it is proper place.

Thu, Mar 14, 3:39 PM · Restricted Project
dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Thu, Mar 14, 1:12 PM · Restricted Project
dblaikie added a comment to D58952: [llvm] Skip over empty line table entries..

Hmm, trying to stare at this function it's confusing me a fair bit. I'd expect this to be more obvious/legible than it is, but it's possible I'm just misunderstanding.

Thu, Mar 14, 1:07 PM · debug-info, Restricted Project
dblaikie added inline comments to D57939: [DWARF] Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries.
Thu, Mar 14, 12:00 PM · Restricted Project
dblaikie added a comment to D58712: Changes for Installing SwiftPM for Apple Swift 5 Toolchain on PowerPC64LE.

@dblaikie, anyone you know who can help here?

Thu, Mar 14, 11:15 AM · Restricted Project
dblaikie added a comment to D59347: [DebugInfo] Combine Trivial and NonTrivial flags.

Would it be simpler/better to revert all the FlagTrivial work? & use the FlagNonTrivial+composite type to imply trivial? (since FlagnonTrivial has been in-tree longer)

Thu, Mar 14, 11:15 AM · Restricted Project

Wed, Mar 13

dblaikie added a comment to D59272: [DebugInfo] Select debug intrinsic line-numbers more carefully when promoting dbg.declare.
In D59272#1426904, @rnk wrote:

This makes me nervous. The pair of the DIVariable and DILocation is used to uniquely identify the variable that a dbg.value applies to. In practice, inlining is usually what creates multiple instances of the same variable that get updated by dbg.values. You can see how the location is used in DwarfDebug::collectVariableInfoFromMFTable.

Last time I checked, the file, line number and column of a dbg.value was completely unused. To uniquely identify and inlined instance of a variable you need to the combination of inlinedAt field of the dbg.value's DILocation and the dbg.value's DILocalVariable.

I see other places in the code that seem to assume that the scope of the location of a dbg.value will get the scope in which the variable is declared. Your change breaks that invariant. I think we could update such code to get the scope from the DIVariable, but maybe that would be wrong since it would lack inlining information.

Isn't the scope coming from the DILocalVariable though?

Wed, Mar 13, 11:57 AM · Restricted Project
dblaikie added a comment to D58712: Changes for Installing SwiftPM for Apple Swift 5 Toolchain on PowerPC64LE.

Any thoughts/inputs on the possible causes?

Wed, Mar 13, 11:53 AM · Restricted Project
dblaikie added inline comments to D59303: [DebugInfo] Pass all values in DebugLocEntry's constructor, NFC.
Wed, Mar 13, 11:38 AM · Restricted Project, debug-info

Tue, Mar 12

dblaikie accepted D58952: [llvm] Skip over empty line table entries..

Great - thanks for your patience!

Tue, Mar 12, 8:15 AM · debug-info, Restricted Project
dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Tue, Mar 12, 8:09 AM · Restricted Project

Mon, Mar 11

dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Mon, Mar 11, 5:48 PM · Restricted Project
dblaikie accepted D59010: [DebugInfo] Add test cases for FlagNonTrivial.

Looks good - thanks!

Mon, Mar 11, 5:42 PM · Restricted Project, Restricted Project
dblaikie committed rGeae78b5157dc: Hexagon RDF: Replace function template (plus explicit specializations) with non… (authored by dblaikie).
Hexagon RDF: Replace function template (plus explicit specializations) with non…
Mon, Mar 11, 4:10 PM
dblaikie committed rL355880: Hexagon RDF: Replace function template (plus explicit specializations) with non….
Hexagon RDF: Replace function template (plus explicit specializations) with non…
Mon, Mar 11, 4:10 PM
dblaikie closed D58998: Replace function template (plus explicit specializations) by non-template overloads..
Mon, Mar 11, 4:10 PM · Restricted Project
dblaikie added inline comments to D58952: [llvm] Skip over empty line table entries..
Mon, Mar 11, 3:37 PM · debug-info, Restricted Project

Sat, Mar 9

dblaikie accepted D58998: Replace function template (plus explicit specializations) by non-template overloads..

Looks good to me, thanks

Sat, Mar 9, 5:13 PM · Restricted Project

Fri, Mar 8

dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Fri, Mar 8, 2:41 PM · Restricted Project
dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Fri, Mar 8, 12:07 PM · Restricted Project

Thu, Mar 7

dblaikie added a comment to D58998: Replace function template (plus explicit specializations) by non-template overloads..

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

The only reason I can think of is that we wanted to keep the overload set small (or trivial), but I don't know if that's significant. (Richard said he doesn't care either way.)

What shall we do now? Would you like to take this change and then refactor, or immediately change this to overloads?

Thu, Mar 7, 1:46 PM · Restricted Project
dblaikie added a comment to D59107: [Support] Add error handling to sys::Process::getPageSize()..

Seems reasonable to me (but feel free to wait for someone with more context/domain awareness)

Thu, Mar 7, 1:23 PM · Restricted Project
dblaikie added a comment to D58952: [llvm] Skip over empty line table entries..

I'm not sure I follow this - the line table maps instructions to source locations. If there are no instructions there's nothing to map

That's the intent. However, in practice it maps instruction *addresses* to source locations.

Well, in practice it's unspecified what it means - so we can look at this and come up with different interpretations. Mine is to interpret these as half open intervals - in which case [100, 100) is empty and you keep searching, then you find [100, 104) and that contains the address you're looking for.

Understood, but if you're walking through the line-number program looking for address 100, and you find it as an exact match, on what grounds do you require the consumer to continue looking?

Thu, Mar 7, 10:34 AM · debug-info, Restricted Project
dblaikie added a comment to D58998: Replace function template (plus explicit specializations) by non-template overloads..

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

Thu, Mar 7, 10:25 AM · Restricted Project

Wed, Mar 6

dblaikie added a comment to D58952: [llvm] Skip over empty line table entries..

I'm not sure I follow this - the line table maps instructions to source locations. If there are no instructions there's nothing to map

That's the intent. However, in practice it maps instruction *addresses* to source locations.

Wed, Mar 6, 4:12 PM · debug-info, Restricted Project
dblaikie added inline comments to D58952: [llvm] Skip over empty line table entries..
Wed, Mar 6, 3:12 PM · debug-info, Restricted Project
dblaikie added inline comments to D58952: [llvm] Skip over empty line table entries..
Wed, Mar 6, 2:44 PM · debug-info, Restricted Project
dblaikie updated subscribers of D58952: [llvm] Skip over empty line table entries..
Wed, Mar 6, 2:02 PM · debug-info, Restricted Project
dblaikie added a comment to D58952: [llvm] Skip over empty line table entries..

Might be worth a separate/targeted test with some of the interesting edge cases? (like empty range at the end of the sequence, etc)

Wed, Mar 6, 9:38 AM · debug-info, Restricted Project
dblaikie added a project to D58952: [llvm] Skip over empty line table entries.: debug-info.
Wed, Mar 6, 9:15 AM · debug-info, Restricted Project
dblaikie added a comment to D59010: [DebugInfo] Add test cases for FlagNonTrivial.

Mind adding at least one example with neither triviality flag? (I guess a type that's not a struct or class? A union maybe?)

Wed, Mar 6, 8:38 AM · Restricted Project, Restricted Project
dblaikie added a comment to D58963: [JumpThreading] Retain debug info when replacing branch instructions.

The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?

Wed, Mar 6, 8:18 AM · Restricted Project

Tue, Mar 5

dblaikie added a comment to D58712: Changes for Installing SwiftPM for Apple Swift 5 Toolchain on PowerPC64LE.

Hi @dblaikie,

While building Apple Swift 5.0 toolchain on PowerPC64LE, we hit up on the following error(details in first comment) while going through the step "--- Installing swiftpm ---" i.e. installation of the swift package manager:-

		swift: /home/sar/swift-source/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:12111: llvm::SDValue combineBVOfConsecutiveLoads(llvm::SDNode *, llvm::SelectionDAG &): Assertion `!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) && "The loads cannot be both consecutive and reverse consecutive."' failed.

The change fixes this issue by correctly calculating the PreviousInput and the NextInput.
Let me know if this is not the correct approach.

Tue, Mar 5, 11:01 AM · Restricted Project

Mon, Mar 4

dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Mon, Mar 4, 11:14 AM · Restricted Project
dblaikie added a comment to D58712: Changes for Installing SwiftPM for Apple Swift 5 Toolchain on PowerPC64LE.

The patch title doesn't seem related (or I don't understand how it's related) to the patch, and the patch description doesn't seem to detail how it relates to the code change.

Mon, Mar 4, 7:40 AM · Restricted Project
dblaikie accepted D58886: [ADT] Add llvm::binary_search to STLExtras.

I guess maybe we have unit testsn for some but not all of these wrappers - might be worth checking/sampling to see what the prevailing trend is (I tend to be over-enthusiastic about testing, but I realize these simple wrappers are pretty uneventful)

Mon, Mar 4, 7:01 AM · Restricted Project

Fri, Mar 1

dblaikie added a comment to D58704: Initial (incomplete) implementation of JITLink - A replacement for RuntimeDyld..

(food for thought, not a "this must be answered now" sort of thing, but "Generic atom-graph data structure and algorithms " does sound a bit like the original LLD design, which didn't turn out to be a great fit (at least the way it ended up) for ELF, at least - any ideas on what might be done differently to avoid the same situation here when adding ELF support down the line? (my naive perspective is that the mismatch was around slicing up ELF sections into MachO-like atoms on symbol boundaries was the mismatch - rather than adding support for an atom with multiple symbols (that may not be at the start of the atom) and then having a one atom to one ELF section mapping))

Fri, Mar 1, 5:09 PM · Restricted Project
dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Fri, Mar 1, 4:39 PM · Restricted Project
dblaikie added inline comments to D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces".
Fri, Mar 1, 4:38 PM · Restricted Project
dblaikie accepted D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism.
Fri, Mar 1, 4:19 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D58849: llvm-dwarfdump: Add new variable, parameter and inlining statistics; also function source location statistics..
Fri, Mar 1, 3:17 PM · Restricted Project
dblaikie added inline comments to D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism.
Fri, Mar 1, 11:15 AM · Restricted Project, Restricted Project
dblaikie accepted D58786: [DebugInfo] Construct nested types on behalf of owner CU.

Looks great - thanks!

Fri, Mar 1, 11:12 AM · Restricted Project
dblaikie added a comment to D58117: Workaround std::thread begin not copy-constructible.
In D58117#1414613, @kcc wrote:
  1. I am not sure what problem does it solve. Everything seems to work.
  2. We can now safely remove fuzzer_allocator and rename Vector to just plain std::vector. (we don't need this monstrosity any more since we are using private STL now)
Fri, Mar 1, 11:09 AM · Restricted Project, Restricted Project

Thu, Feb 28

dblaikie added inline comments to D58750: [DWARF] Make -g with empty assembler source work better..
Thu, Feb 28, 3:56 PM · Restricted Project, debug-info
dblaikie added a comment to D58786: [DebugInfo] Construct nested types on behalf of owner CU.

Would it be possible/practical/reasonable to split getOrCreateTypeDIE at 643, before the createAndAddDIE? And at that point do the switch to the appropriate CU (basically as soon as we get the context DIE and can determine which unit is the appropriate one to build the type DIE in)

Thu, Feb 28, 3:31 PM · Restricted Project
dblaikie accepted D58520: [Subtarget] Remove static global constructor call from the tablegened subtarget feature tables.
Thu, Feb 28, 3:29 PM · Restricted Project
dblaikie added a comment to D58787: [ProfileData] Sort ProfilingData by hash.

How about we keep the discussion on the original patch (D57986)? I think we explored the problem space a fair bit there & I'll advocate for the same thing here as I did there: Sorting before emission is likely more efficient than keeping a continuously sorted container, if there's a separate "build" and "iterate" phase you can sort between. At least that's my understanding. std::map and MapVector would both grow memory usage, probably not prohibitively, but a fair bit.

Thu, Feb 28, 1:29 PM · Restricted Project

Wed, Feb 27

dblaikie added inline comments to D58406: Fix IR/Analysis layering issue in OptBisect.
Wed, Feb 27, 11:19 AM · Restricted Project

Tue, Feb 26

dblaikie added a comment to D58538: [DebugInfo] Add source attributes for function declaration on behalf of owner CU.

I'll test a couple of related cases (implicit special members and nested classes) here in a moment & see if they have any related bugs.

Tue, Feb 26, 4:33 PM · Restricted Project
dblaikie accepted D58194: [DebugInfo] add SectionedAddress to DebugInfo interfaces..

Looks good - thanks!

Tue, Feb 26, 4:20 PM · debug-info, Restricted Project
dblaikie accepted D58538: [DebugInfo] Add source attributes for function declaration on behalf of owner CU.

Here's a bit of a smaller test that might make it clearer (could add some comments to help explain why each part is present):

Tue, Feb 26, 4:16 PM · Restricted Project
dblaikie added inline comments to D58698: [DWARFFormValue] Don't consider DW_FORM_data4/8 to be section offsets..
Tue, Feb 26, 3:06 PM · Restricted Project
dblaikie added inline comments to D58698: [DWARFFormValue] Don't consider DW_FORM_data4/8 to be section offsets..
Tue, Feb 26, 2:55 PM · Restricted Project

Mon, Feb 25

dblaikie added inline comments to D58538: [DebugInfo] Add source attributes for function declaration on behalf of owner CU.
Mon, Feb 25, 5:15 PM · Restricted Project
dblaikie added inline comments to D58194: [DebugInfo] add SectionedAddress to DebugInfo interfaces..
Mon, Feb 25, 2:45 PM · debug-info, Restricted Project
dblaikie added a comment to D58628: [cmake] Add option to enable gdb-index..

Would it be simpler/effective to add "-fuse-ld=gold -Wl,--gdb-index" to your linker flags to enable both these things, rather than having specific CMake support for either/both?

Mon, Feb 25, 12:43 PM · Restricted Project
dblaikie added a comment to D58628: [cmake] Add option to enable gdb-index..

Hmm, I don't seem to have had the troubles you've had - I use CMAKE_EXE_LINKER_FLAGS_DEBUG:STRING=-Wl,--gdb-index and... ah, I don't use (or even have in my CMakeCache.txt any mention of) LLVM_USE_LINKER. Not actually sure where/how my build is choosing to use gold. Perhaps it's just my system default once it's installed.

Mon, Feb 25, 12:35 PM · Restricted Project

Feb 22 2019

dblaikie added a comment to D56587: Introduce DW_OP_LLVM_convert.

I don't think anyone mentioned it, that's why I was bringing it up - there's always an option to not render any location if it's not possible/worth the work. That's all I was asking - is it worth the complexity? (I wasn't sure anyone needed it - but sounds like Sony does, reckon it's worth the tradeoff in complexity in LLVM compared to the work required to support this in the Sony debugger?)

NVPTX also would need it, because they are stuck on DWARF v2.

Any ideas if NVPTX hit this case? my understanding was that NVPTX has a fairly restrictive set of code or actions that can be used.

Feb 22 2019, 3:53 PM · Restricted Project, debug-info
dblaikie added a comment to D58538: [DebugInfo] Add source attributes for function declaration on behalf of owner CU.

I think the point where we lookup the other CU and cross over to it should probably be earlier - imagine if all attributes could potentially be CU-local (eg: we could produce a separate string_offsets section for each CU and so have separate strx indexes for each CU) & we should probably have a solution that would be correct in that situation. Does that make sense?

Feb 22 2019, 2:29 PM · Restricted Project

Feb 21 2019

dblaikie added a comment to D58238: [DebugInfo] MachineSink: Insert undef DBG_VALUEs when sinking instructions, try to forward copies.

So this basically prevents the debugger from displaying an inconsistent program state when some of the sideeffects from a line preceding its current stopping point have not been completed yet. Makes sense to me. One minor concern: If the sunk instruction does not get sunk past the next source line, we may create some unneeded location list entries, e.g.:

x = y; x = x + 3; z = z ? z + 3 : b; // all on the same line

If the x + 3 is sunk past the assignment to z (but before any code attributed to the next line is executed) we generate an additional location list entry for x, where there is a small gap in the covered range for x, even though the user is not able to observe x until the next line.
Doesn't look like a big deal, though.

Feb 21 2019, 3:48 PM · Restricted Project
dblaikie added a comment to D58088: [adt] Add raw_pointer_iterator to iterate over std::unique_ptr<> collections.

That looks simple enough. I haven't gone through the spec but something as simple as:

namespace llvm {
template<class Ptr>
auto to_address(const Ptr &P) noexcept { return P.operator->(); }
template<class T>
constexpr T* to_address(T* P) noexcept { return P; }
}

seems to cover the majority of it (and definitely the bits I need). It's just the std::pointer_traits<Ptr>::to_address(p) based implementation of the fancy pointer overload that's missing.

If it is named to_address and intended to convert any pointer to a raw pointer then it should be:

namespace llvm {
template<class T>
constexpr T* to_address(T* p) noexcept { return p; }
template<class Ptr>
typename std::pointer_traits<Ptr>::element_type* to_address(const Ptr &p) noexcept { return llvm::to_address(p.operator->()); }
}

Well spotted. I'm about to revert the patch though as I've found a way of doing this without using C++20 or defining any new classes that's good enough (because there's no nullptrs) for the GISel Combiner code that wants to use this

Feb 21 2019, 1:30 PM · Restricted Project
dblaikie added inline comments to D57939: [DWARF] Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries.
Feb 21 2019, 9:52 AM · Restricted Project
dblaikie added inline comments to D58421: Add partial implementation of std::to_address() as llvm::to_address().
Feb 21 2019, 9:24 AM · Restricted Project
dblaikie added a comment to D58406: Fix IR/Analysis layering issue in OptBisect.

Last time, this was brought up, passing strings was deemed too expensive since it would cause a string to be created every time:
http://lists.llvm.org/pipermail/llvm-dev/2018-March/122032.html

There's a difference between getting the pass name and the IR unit name. Pass names are hard-coded string literals which can be cheaply referred to by a StringRef. IR units need to create a new string with the description (except Loop and Region, which don't have interesting descriptions yet.)

Feb 21 2019, 9:23 AM · Restricted Project

Feb 20 2019

dblaikie added a comment to D58088: [adt] Add raw_pointer_iterator to iterate over std::unique_ptr<> collections.

I just noticed a big simplification that doesn't need C++20 features or new classes. This:

template <typename WrappedIteratorT,
          typename T1 = typename std::remove_reference<decltype(**std::declval<WrappedIteratorT>())>::type,
          typename T2 = typename std::add_pointer<T1>::type>
using raw_pointer_iterator = pointer_iterator<pointee_iterator<WrappedIteratorT, T1>, T2>;

is functionally equivalent to the raw_pointer_iterator in this patch. pointee_iterator derefs the unique_ptr, while pointer_iterator produces the raw_pointer. If that version sounds good to you then I think it makes sense to revert the llvm::to_address() patch, switch to this implementation and fold this into the GISel Combiner patch as at that point it's too trivial to warrant a dedicated patch. Does that sound good?

Feb 20 2019, 2:07 PM · Restricted Project
dblaikie accepted D58421: Add partial implementation of std::to_address() as llvm::to_address().

Looks good - thanks!

Feb 20 2019, 10:04 AM · Restricted Project
dblaikie added a comment to D58406: Fix IR/Analysis layering issue in OptBisect.

Alternatively - how would you feel about callers passing in the strings directly?

return !BisectEnabled || checkPass(P->getPassName(), P->getDescription(U));

The Pass and Unit are only used to retrieve the Pass Name and Description (passing the Unit straight back to the Pass that just passed it in) - so why not pass in those (Pass Name and Description) instead?

You mean e.g. changing

!F.getContext().getOptPassGate().shouldRunPass(this, &F))

into

!F.getContext().getOptPassGate().shouldRunPass(this->getName(), F.getDescription()))

Yep, that would resolve the layering issue w/o requiring opaque pointers.

Btw, in new-pm's pass instrumentation we do pass Pass Name instead of a pointer to pass exactly because of this layering issue.
However, note, that there we still pass IR unit itself (wrapped into llvm::Any) instead of "description"
because instrumentations framework is generic and instrumentation functionality is not predefined.
And yet, specific instrumentations like PrintIR do need IRUnit's description and right now every instrumentation does it on its own.

If we could implement a generic way of querying IRUnit description (w/o introducing a layering issue), it would
be beneficial both for legacy and new pm implementations.

Feb 20 2019, 8:26 AM · Restricted Project
dblaikie added a comment to D58406: Fix IR/Analysis layering issue in OptBisect.

The need for generic way of getting textual description for IRUnit is obvious both in legacy and new pass manager.
However in this particular approach I dont like how "const void*" is used to erase specific type of IRUnit.

In new pass manager there was a similar task of erasing the type of IRUnit for pass-instrumentation purposes.
And there we decided to wrap the pointer into llvm::Any.

I'm not sure llvm::Any is the best approach here, but it sounds better than absolute anarchy of const void*.

Feb 20 2019, 7:54 AM · Restricted Project

Feb 19 2019

dblaikie added inline comments to D58421: Add partial implementation of std::to_address() as llvm::to_address().
Feb 19 2019, 9:03 PM · Restricted Project
dblaikie added reviewers for D58406: Fix IR/Analysis layering issue in OptBisect: fedor.sergeev, andrew.w.kaylor, efriedma.
Feb 19 2019, 3:07 PM · Restricted Project
dblaikie added a comment to D58117: Workaround std::thread begin not copy-constructible.

Fedora is currently using gcc-9.0.1, svn revision 268719 (see https://src.fedoraproject.org/rpms/gcc/blob/master/f/gcc.spec) which triggers this issue. I'll forward the test case to the gcc team and report here.

Feb 19 2019, 11:31 AM · Restricted Project, Restricted Project
dblaikie added a comment to D56587: Introduce DW_OP_LLVM_convert.

I'm a touch confused by the whole discussion here - so I'll write some things & perhaps folks can correct my understanding where necessary.

Feb 19 2019, 11:18 AM · Restricted Project, debug-info

Feb 18 2019

dblaikie added a comment to D58088: [adt] Add raw_pointer_iterator to iterate over std::unique_ptr<> collections.

Hmm, It might be nice to have this as a specialization of pointer_iterator if possible.

I think it might be a source of confusion/frustration to have both a raw_pointer_iterator and a pointer_iterator, where raw_pointer_iterator is specifically used for only unique_ptr.

I can see a way to achieve that (std::to_address()) but it requires C++20. Without that, the closest I've been able to get is a raw_pointer_iterator alias to pointer_iterator that has specializations based on whether a third optional argument is true_type or false_type. The main thing I'm missing to fold the alias in is a way to specify the default value of T in a way that resolves to either pointer_iterator's default type T or raw_pointer_iterators default type T depending on whether *Iter is a std::unique_ptr<X>& or not. I can detect the type easily enough but selecting between the two defaults is proving troublesome as it resolves the true and false cases too early and ends up trying to do int::pointer and failing because int isn't a class type.

Would it be difficult to have LLVM's own version of std::to_address (LLVM has had/does have various type traits implmented in-tree when they aren't available in the standard LLVM's using at the moment) or similar to use here?

That looks simple enough. I haven't gone through the spec but something as simple as:

namespace llvm {
template<class Ptr>
auto to_address(const Ptr &P) noexcept { return P.operator->(); }
template<class T>
constexpr T* to_address(T* P) noexcept { return P; }
}

seems to cover the majority of it (and definitely the bits I need). It's just the std::pointer_traits<Ptr>::to_address(p) based implementation of the fancy pointer overload that's missing.

Feb 18 2019, 5:47 PM · Restricted Project
dblaikie added inline comments to D57939: [DWARF] Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries.
Feb 18 2019, 4:22 PM · Restricted Project
dblaikie added inline comments to D57940: Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries.
Feb 18 2019, 3:59 PM · Restricted Project
dblaikie added a comment to D58088: [adt] Add raw_pointer_iterator to iterate over std::unique_ptr<> collections.

Hmm, It might be nice to have this as a specialization of pointer_iterator if possible.

I think it might be a source of confusion/frustration to have both a raw_pointer_iterator and a pointer_iterator, where raw_pointer_iterator is specifically used for only unique_ptr.

I can see a way to achieve that (std::to_address()) but it requires C++20. Without that, the closest I've been able to get is a raw_pointer_iterator alias to pointer_iterator that has specializations based on whether a third optional argument is true_type or false_type. The main thing I'm missing to fold the alias in is a way to specify the default value of T in a way that resolves to either pointer_iterator's default type T or raw_pointer_iterators default type T depending on whether *Iter is a std::unique_ptr<X>& or not. I can detect the type easily enough but selecting between the two defaults is proving troublesome as it resolves the true and false cases too early and ends up trying to do int::pointer and failing because int isn't a class type.

Feb 18 2019, 3:52 PM · Restricted Project
dblaikie added a comment to D58117: Workaround std::thread begin not copy-constructible.

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

Right! https://godbolt.org/z/c3iMsT proves you right. The part that actually matters seems to be the `construct` member forwarding (why is inheritance not enough in that case?).

Feb 18 2019, 3:41 PM · Restricted Project, Restricted Project
dblaikie added a comment to D58117: Workaround std::thread begin not copy-constructible.

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

Feb 18 2019, 12:22 PM · Restricted Project, Restricted Project

Feb 14 2019

dblaikie added inline comments to D57939: [DWARF] Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries.
Feb 14 2019, 11:57 AM · Restricted Project
dblaikie added a comment to D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism.
In D57986#1398271, @vsk wrote:

I think this could roughly double the memory utilization of the writer, which might be problematic because the number of records to write can be high. (@dblaikie did some work on reducing memory usage in this area, he might have hard data about this.)

As the write should only occur once, maybe the better tradeoff would be to sort?

Feb 14 2019, 11:46 AM · Restricted Project, Restricted Project

Feb 13 2019

dblaikie added a comment to D58020: [exposition-only] Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries.

How does this relate to D57940?

Feb 13 2019, 12:38 PM · Restricted Project
dblaikie added a comment to D58194: [DebugInfo] add SectionedAddress to DebugInfo interfaces..

This has some relatively extensive changes to some of the llvm tools (like sancov, for instance) that aren't tested - perhaps it'd be better to leave those tools usage unmodified (adding the undef section value as a fix to the API change, but nothing more) with fixits and/or followup patches that implement the improved functionality possible with the new API, along with tests?

Feb 13 2019, 12:31 PM · debug-info, Restricted Project