Page MenuHomePhabricator

dsanders (Daniel Sanders)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 19 2013, 3:30 PM (433 w, 2 d)

Recent Activity

Wed, Dec 1

dsanders committed rG54e21df973e1: [unroll] Fix a functional change in an NFC patch (authored by dsanders).
[unroll] Fix a functional change in an NFC patch
Wed, Dec 1, 5:28 PM
dsanders closed D114894: [unroll] Fix a functional change in an NFC patch.
Wed, Dec 1, 5:28 PM · Restricted Project
dsanders added a comment to D114894: [unroll] Fix a functional change in an NFC patch.

Do you have a test case by any chance that would show the functional change?

Wed, Dec 1, 12:48 PM · Restricted Project
dsanders requested review of D114898: [mipatternmatch] Document the mi_match() parameters.
Wed, Dec 1, 11:46 AM · Restricted Project
dsanders requested review of D114894: [unroll] Fix a functional change in an NFC patch.
Wed, Dec 1, 11:19 AM · Restricted Project

Tue, Nov 30

dsanders committed rG9e3e1aad3161: [InstCombine] Allow fake vector insert folding to bit-logic only if the insert… (authored by Srividya-Karumuri).
[InstCombine] Allow fake vector insert folding to bit-logic only if the insert…
Tue, Nov 30, 1:55 PM
dsanders closed D114734: [InstCombine] Allow fake vector insert folding to bit-logic only if the insert element is integer type.
Tue, Nov 30, 1:55 PM · Restricted Project

Tue, Nov 16

dsanders added a comment to D25618: Check that emitted instructions meet their predicates on all targets except ARM, Mips, and X86..

Sorry for commenting on such an old patch but I am wondering why was verifyInstructionPredicates implemented inside of MCCodeEmitter? In practice this seem to mean that llc -filetype=obj gets this checking but llc -filetype=asm (the default, as used by almost all lit codegen tests) does not. Would it be possible to move verifyInstructionPredicates somewhere a bit more generic so that both paths can get this checking?

Tue, Nov 16, 11:32 AM · Restricted Project

Oct 14 2021

dsanders committed rG0a869ef3a844: [llvm-mca][timeline] Indicate output was stopped due to cycle limit. (authored by dsanders).
[llvm-mca][timeline] Indicate output was stopped due to cycle limit.
Oct 14 2021, 11:10 AM
dsanders closed D111753: [llvm-mca][timeline] Indicate output was stopped due to cycle limit..
Oct 14 2021, 11:10 AM · Restricted Project

Oct 13 2021

dsanders requested review of D111753: [llvm-mca][timeline] Indicate output was stopped due to cycle limit..
Oct 13 2021, 12:13 PM · Restricted Project

Oct 4 2021

dsanders added inline comments to D109750: [GlobalISel][Legalizer] Don't use eraseFromParentAndMarkDBGValuesForRemoval() for some artifacts..
Oct 4 2021, 2:10 PM · Restricted Project

Sep 29 2021

dsanders accepted D110747: [NFC] Restore magic and magicu to a globally visible location.

LGTM

Sep 29 2021, 11:43 AM · Restricted Project

Sep 15 2021

dsanders added inline comments to D109476: [APInt.h] Reduce the APInt header file interface a bit. NFC.
Sep 15 2021, 1:26 PM · Restricted Project

Jul 9 2021

dsanders added a comment to rGe892705d74c7: GlobalISel: Do not change register types in lowerLoad.

Is anyone already looking into supporting narrowing the result type below the memory size?

With this change, we cannot legalize %1:_(s128) = G_ZEXTLOAD %0:_(p0) :: (load (s64)) anymore, as s32 is the only legal result type for G_ZEXTLOAD on our target.

This is not a lowering, this is a narrowScalar of the result type. I'm trying to move towards narrowScalar never touching the memory type

I'm confused by this. You can't narrowScalar this below the memory type without splitting into multiple loads with smaller memory types since none of the load instructions are allowed to be truncating.

You can narrowScalar to a regular s64 load + zext. Whatever happens to that 64-bit load is a separate question

Jul 9 2021, 10:41 AM
dsanders added a comment to rGe892705d74c7: GlobalISel: Do not change register types in lowerLoad.

Is anyone already looking into supporting narrowing the result type below the memory size?

With this change, we cannot legalize %1:_(s128) = G_ZEXTLOAD %0:_(p0) :: (load (s64)) anymore, as s32 is the only legal result type for G_ZEXTLOAD on our target.

This is not a lowering, this is a narrowScalar of the result type. I'm trying to move towards narrowScalar never touching the memory type

Jul 9 2021, 10:28 AM
dsanders added a comment to rGe892705d74c7: GlobalISel: Do not change register types in lowerLoad.

Is anyone already looking into supporting narrowing the result type below the memory size?

With this change, we cannot legalize %1:_(s128) = G_ZEXTLOAD %0:_(p0) :: (load (s64)) anymore, as s32 is the only legal result type for G_ZEXTLOAD on our target.

Jul 9 2021, 10:15 AM

Jun 15 2021

dsanders added a comment to D96670: [CMake] Look up target subcomponents in LLVM_AVAILABLE_LIBS.

I'm not familiar with the behaviour of cmake exports so I don't feel able to review this but FWIW, since it goes on to check libraries against LLVM_AVAILABLE_LIBS line 289 it doesn't sound wrong to check against that a little earlier.

Jun 15 2021, 10:16 AM · Restricted Project

Jun 4 2021

dsanders accepted D103684: [AMDGPU] Stop using LegacyLegalizerInfo. NFCI..

LGTM, Thanks!

Jun 4 2021, 10:52 AM · Restricted Project

Jun 2 2021

dsanders accepted D97791: [GlobalISel] Handle non-multiples of the base type in narrowScalarInsert.

LGTM (we should wait for Matt to confirm he's ok with dealing with the high-level issue later though)

Jun 2 2021, 11:42 AM · Restricted Project

Jun 1 2021

dsanders added inline comments to rGe892705d74c7: GlobalISel: Do not change register types in lowerLoad.
Jun 1 2021, 2:49 PM
dsanders committed rG937266205026: fixup: Missing operator in [globalisel][legalizer] Separate the deprecated… (authored by dsanders).
fixup: Missing operator in [globalisel][legalizer] Separate the deprecated…
Jun 1 2021, 1:58 PM
dsanders added inline comments to rGe892705d74c7: GlobalISel: Do not change register types in lowerLoad.
Jun 1 2021, 1:53 PM
dsanders committed rGaaac268285ff: [globalisel][legalizer] Separate the deprecated LegalizerInfo from the current… (authored by dsanders).
[globalisel][legalizer] Separate the deprecated LegalizerInfo from the current…
Jun 1 2021, 1:24 PM
dsanders closed D103352: [globalisel][legalizer] Separate the deprecated LegalizerInfo from the current one.
Jun 1 2021, 1:24 PM · Restricted Project
dsanders added a comment to D103352: [globalisel][legalizer] Separate the deprecated LegalizerInfo from the current one.

How much work is left to just remove this completely?

Jun 1 2021, 11:09 AM · Restricted Project

May 28 2021

dsanders requested review of D103352: [globalisel][legalizer] Separate the deprecated LegalizerInfo from the current one.
May 28 2021, 7:38 PM · Restricted Project
dsanders added inline comments to rGe892705d74c7: GlobalISel: Do not change register types in lowerLoad.
May 28 2021, 3:19 PM

May 19 2021

dsanders accepted D102754: [unittests][CodeGen] Mark tests that cannot be executed with GTEST_SKIP().

LGTM too. Thanks!

May 19 2021, 2:29 PM · Restricted Project

May 5 2021

dsanders added a comment to D101187: [MachineCSE] Prevent CSE of non-local convergent instrs.

LGTM

May 5 2021, 1:14 PM · Restricted Project

Apr 29 2021

dsanders accepted D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns.

LGTM. Sorry for the long wait

Apr 29 2021, 5:50 PM · Restricted Project
dsanders accepted D101187: [MachineCSE] Prevent CSE of non-local convergent instrs.

LGTM with the check in ProcessBlockPRE

Apr 29 2021, 9:57 AM · Restricted Project

Apr 26 2021

dsanders added inline comments to D101187: [MachineCSE] Prevent CSE of non-local convergent instrs.
Apr 26 2021, 3:01 PM · Restricted Project
dsanders added inline comments to D101187: [MachineCSE] Prevent CSE of non-local convergent instrs.
Apr 26 2021, 2:58 PM · Restricted Project

Apr 23 2021

dsanders requested changes to D101187: [MachineCSE] Prevent CSE of non-local convergent instrs.
Apr 23 2021, 6:17 PM · Restricted Project
dsanders accepted D101187: [MachineCSE] Prevent CSE of non-local convergent instrs.
Apr 23 2021, 4:15 PM · Restricted Project
dsanders added a comment to D101187: [MachineCSE] Prevent CSE of non-local convergent instrs.

The change LGTM but could you add a test case? There probably aren't many convergent instructions upstream but it should be possible to make a test case using AMDGPU instructions or via G_INTRINSIC

Apr 23 2021, 11:50 AM · Restricted Project

Apr 22 2021

dsanders accepted D101050: [GlobalISel] Remove ConstantFoldingMIRBuilder.

I'm not aware of anything still using it either. LGTM

Apr 22 2021, 11:33 AM · Restricted Project

Apr 19 2021

dsanders committed rG76b0ea7f2d5c: Reset NextFnNum in MachineModuleInfo::initialize (authored by rtereshin).
Reset NextFnNum in MachineModuleInfo::initialize
Apr 19 2021, 3:52 PM
dsanders closed D100797: Reset NextFnNum in MachineModuleInfo::initialize.
Apr 19 2021, 3:52 PM · Restricted Project
dsanders added a comment to D100797: Reset NextFnNum in MachineModuleInfo::initialize.

It turns out someone already added C++11 default initializers but they're not used in this case. MachineModuleInfo could use some better commenting about its lifetime so I'll add that in a follow-up at the same time as renaming it

Apr 19 2021, 3:11 PM · Restricted Project
dsanders added a comment to D100797: Reset NextFnNum in MachineModuleInfo::initialize.

Patch by Roman Tereshin (it seems arcanist loses that info for the review)

Apr 19 2021, 2:52 PM · Restricted Project
dsanders requested review of D100797: Reset NextFnNum in MachineModuleInfo::initialize.
Apr 19 2021, 2:48 PM · Restricted Project
dsanders requested review of D100787: [globalisel][unittest] Better error reporting when unittests fail.
Apr 19 2021, 12:38 PM · Restricted Project

Apr 13 2021

dsanders committed rGbe50657c6ac5: [TableGen] Resolve concrete but not complete field access initializers (authored by dsanders).
[TableGen] Resolve concrete but not complete field access initializers
Apr 13 2021, 3:15 PM
dsanders closed D100253: [TableGen] Resolve concrete but not complete field access initializers.
Apr 13 2021, 3:15 PM · Restricted Project

Apr 12 2021

dsanders added a comment to D100253: [TableGen] Resolve concrete but not complete field access initializers.

I just pushed my bug fix. Let's wait a day until I know it doesn't bust the build, then push this one.

Sounds good to me, thanks. I've also unreverted the original downstream and applied your fix there too.

Apr 12 2021, 3:07 PM · Restricted Project

Apr 11 2021

dsanders added a comment to D100253: [TableGen] Resolve concrete but not complete field access initializers.

Yep, ./bin/llvm-lit -svv ../llvm-project/llvm/test/TableGen and ninja check passes

Apr 11 2021, 4:08 PM · Restricted Project

Apr 10 2021

dsanders accepted D100254: Typo fix.

LGTM

Apr 10 2021, 10:37 PM · Restricted Project
dsanders added inline comments to D100253: [TableGen] Resolve concrete but not complete field access initializers.
Apr 10 2021, 8:08 PM · Restricted Project
dsanders added a comment to D100247: [TableGen] Fix bug in recent change to ListInit::convertInitListSlice().

The patches I mentioned are in D100253

Apr 10 2021, 8:04 PM · Restricted Project
dsanders requested review of D100253: [TableGen] Resolve concrete but not complete field access initializers.
Apr 10 2021, 8:03 PM · Restricted Project
dsanders added a comment to D100247: [TableGen] Fix bug in recent change to ListInit::convertInitListSlice().

I'll post a patch for the Rec10.Zero fix in a moment

Apr 10 2021, 7:21 PM · Restricted Project

Apr 9 2021

dsanders added a comment to rG14580ce2fdd1: [TableGen] Make behavior of list slice suffix consistent across all values.

Ah, okay, that sounds reasonable. Now what is the circumstance under which ? is coerced to 0?

Apr 9 2021, 6:40 PM
dsanders added a comment to rG14580ce2fdd1: [TableGen] Make behavior of list slice suffix consistent across all values.

@dsanders

Okay, there is a lot going on here.

If you look at the two lines I added to Record.cpp, you'll see that I did an awesome job of it. It always fetches element 0 of the list, regardless of the actual subscript. So the last two fields in Z7LstBits will have element 0, the ?.

Apr 9 2021, 5:14 PM
dsanders added a comment to rG14580ce2fdd1: [TableGen] Make behavior of list slice suffix consistent across all values.

You're right the bit slices haven't changed behaviour, the one that has is list slices. Sorry about that, the same test has bit slices nearby and it's trying to test bit/list slices are consistent w.r.t our !iscomplete() predicate.

Apr 9 2021, 2:15 PM

Apr 8 2021

dsanders added a comment to rG14580ce2fdd1: [TableGen] Make behavior of list slice suffix consistent across all values.

Hi Paul,

Apr 8 2021, 10:29 PM
dsanders accepted D100152: Support: Drop the no-op initializer for SignpostEmitterImpl::Signposts, NFC.

LGTM

Apr 8 2021, 4:33 PM · Restricted Project
dsanders accepted D100154: Support: Avoid unnecessary std::function for SignpostEmitterImpl::SignpostLog.

LGTM

Apr 8 2021, 4:32 PM · Restricted Project
dsanders accepted D100151: Support: Use std::unique_ptr for SignpostEmitter::Impl, NFC.

LGTM

Apr 8 2021, 4:31 PM · Restricted Project

Apr 1 2021

dsanders added a comment to D99692: [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests.

Looks like phabricator noticed the revert but not the recommit. Here's the recommit:

bc21c7baaf00 [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests

I forgot to update my local commit message to the phabricator one before committing the first time around

Apr 1 2021, 4:52 PM · Restricted Project
dsanders added a reverting change for rG3a016e31ecef: [globalisel][unittests] Rename setUp() to avoid potential mix up with SetUp()…: rG42a84d22c4e0: Revert "[globalisel][unittests] Rename setUp() to avoid potential mix up with….
Apr 1 2021, 4:49 PM
dsanders committed rG42a84d22c4e0: Revert "[globalisel][unittests] Rename setUp() to avoid potential mix up with… (authored by dsanders).
Revert "[globalisel][unittests] Rename setUp() to avoid potential mix up with…
Apr 1 2021, 4:49 PM
dsanders added a reverting change for D99692: [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests: rG42a84d22c4e0: Revert "[globalisel][unittests] Rename setUp() to avoid potential mix up with….
Apr 1 2021, 4:49 PM · Restricted Project
dsanders accepted D99776: [MIPS, test] Fix use of undef FileCheck var.

LGTM

Apr 1 2021, 4:46 PM · Restricted Project
dsanders committed rG3a016e31ecef: [globalisel][unittests] Rename setUp() to avoid potential mix up with SetUp()… (authored by dsanders).
[globalisel][unittests] Rename setUp() to avoid potential mix up with SetUp()…
Apr 1 2021, 4:42 PM
dsanders closed D99692: [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests.
Apr 1 2021, 4:42 PM · Restricted Project

Mar 31 2021

dsanders retitled D99692: [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests from [globalisel][unittests] Rename setUp() to avoid potential mix up with SetUp() from gtest to [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests.
Mar 31 2021, 5:07 PM · Restricted Project
dsanders added a comment to D99692: [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests.

The title/summary seems a bit confusing. The interesting part of this change is having the set up function return a target machine so it's impossible to miss - IMO renaming it is incidental. This seems reasonable to do, but I think it'd be better titled something along the lines of "Return a target machine from the test setup so that it's structurally required"

Mar 31 2021, 4:59 PM · Restricted Project
dsanders requested review of D99692: [globalisel][unittests] Make it a compiler error to not call setUp() to avoid future rotten green tests.
Mar 31 2021, 4:36 PM · Restricted Project

Mar 30 2021

dsanders added a comment to D95257: [RGT][GlobalISel] Add missing setUp() calls to legalizer unittests.

Thanks for fixing this, I just ran into the same problem after basing a new test off of one of these on an older branch and wondering why it passed before I'd written the result string. I did a bit of digging before finding this commit and I think part of the problem here is that gtest will call the virtual function SetUp() automatically but not setUp(). I think we should also rename setUp() to something that doesn't clash with gtest (createTargetAndModule, initLLVM, etc.)

Mar 30 2021, 8:14 PM · Restricted Project
dsanders added a comment to D74588: Make TableGenGlobalISel an object library.

All I wanted was a subdirectory to organize the sources specific to the GISel tablegen passes :-).

Not only that, you also want to use them in TableGenTests, and this I think is why can't just have them in llvm-tblgen.

Mar 30 2021, 7:45 PM · Restricted Project
dsanders added a comment to D74588: Make TableGenGlobalISel an object library.

Yep, and that's because of a different bug in how LLVMTableGenGlobalISel was setup.

This patch would address the issues: [...]

Ok, that allows me to remove the DISABLE_LLVM_LINK_LLVM_DYLIB, but

  • maybe it would be confusing to have an LLVM component in llvm/utils instead of llvm/lib, and
  • maybe this shouldn't be part of LLVM because it's only needed for llvm-tblgen? In other words maybe this isn't actually a component of LLVM proper but just a tool for building LLVM. If we make it a component library it will be part of libLLVM.so.
Mar 30 2021, 1:07 PM · Restricted Project

Mar 29 2021

dsanders added a comment to D74588: Make TableGenGlobalISel an object library.

I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.

The commit message of rL373651 says

Fixed the -DLLVM_LINK_LLVM_DYLIB=ON using DISABLE_LLVM_LINK_LLVM_DYLIB when creating the library. Apparently it automatically links to libLLVM.dylib and we don't want that from tablegen.

This makes no sense. The tablegen tool build disables linking the dylib, which should be sufficient. We should never need to force it on a static archive. I’m 99% sure, that rL373651 is just wrong here, and that the solution to all of this is to just remove DISABLE_LLVM_LINK_LLVM_DYLIB from the library.

Mar 29 2021, 7:19 PM · Restricted Project
dsanders added inline comments to D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns.
Mar 29 2021, 2:10 PM · Restricted Project

Mar 23 2021

dsanders added inline comments to D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns.
Mar 23 2021, 1:08 PM · Restricted Project
dsanders added inline comments to D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns.
Mar 23 2021, 12:28 PM · Restricted Project
dsanders added a comment to D99033: GlobalISel: Constant fold G_PTR_ADD.

Relaxing G_ADD semantics seems fine to me. Maybe we should still canonicalize on G_PTR_ADD during translation, and allow targets to legalize them into plain G_ADDs.

Mar 23 2021, 10:53 AM · Restricted Project

Mar 16 2021

dsanders added inline comments to D98046: [TableGen] Fix excessive compile time issue in FixedLenDecoderEmitter.
Mar 16 2021, 1:25 PM · Restricted Project
dsanders accepted D98046: [TableGen] Fix excessive compile time issue in FixedLenDecoderEmitter.

LGTM

Mar 16 2021, 12:22 PM · Restricted Project
dsanders added a comment to D98046: [TableGen] Fix excessive compile time issue in FixedLenDecoderEmitter.

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

Sorry, I've gone to try and confirm this a couple times in the last week and things kept coming up. I'll take another look a moment

Mar 16 2021, 12:13 PM · Restricted Project
dsanders added a comment to D98046: [TableGen] Fix excessive compile time issue in FixedLenDecoderEmitter.

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

Mar 16 2021, 11:19 AM · Restricted Project

Mar 10 2021

dsanders added inline comments to D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns.
Mar 10 2021, 5:30 PM · Restricted Project
dsanders added inline comments to D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns.
Mar 10 2021, 5:21 PM · Restricted Project
dsanders committed rG134a179dee87: [mir] Change 'undef' for MMO base addresses to 'unknown-address' (authored by dsanders).
[mir] Change 'undef' for MMO base addresses to 'unknown-address'
Mar 10 2021, 4:47 PM
dsanders closed D98100: [mir] Change 'undef' for MMO base addresses to 'unknown-address'.
Mar 10 2021, 4:47 PM · Restricted Project
dsanders added inline comments to D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns.
Mar 10 2021, 4:05 PM · Restricted Project

Mar 5 2021

dsanders requested review of D98100: [mir] Change 'undef' for MMO base addresses to 'unknown-address'.
Mar 5 2021, 8:47 PM · Restricted Project
dsanders added a comment to rG955365524aee: [MCParser] Bring back srcmanager diagnostics in AsmParser.

I was able to switch our downstream tools to use the MCContext diagnostic handler. For one case I added an empty SourceMgr when the MCContext was created, if you don't do that then SrcMgr can be nullptr even though it's a reference. For the other, I had a little trouble with MCContext's handler being able to store some opaque data and give it back when it calls the handler but I was able to resolve this by wrapping my handler in a capturing lambda and capturing the variable I needed.

Mar 5 2021, 10:04 AM

Mar 4 2021

dsanders committed rG9fc2be6f289e: [mir] Fix confusing MIR when MMO's value is nullptr but offset is non-zero (authored by dsanders).
[mir] Fix confusing MIR when MMO's value is nullptr but offset is non-zero
Mar 4 2021, 10:35 AM
dsanders closed D97812: [mir] Fix confusing MIR when MMO's value is nullptr but offset is non-zero.
Mar 4 2021, 10:35 AM · Restricted Project
dsanders accepted D97942: [TableGen] Fix warning when compiling generated MCCodeEmitter.

LGTM

Mar 4 2021, 10:15 AM · Restricted Project

Mar 3 2021

dsanders added a comment to rG955365524aee: [MCParser] Bring back srcmanager diagnostics in AsmParser.

It looks like I have a second problem hiding behind the first which I'm not sure how to solve yet. It only appears in our downstream tests because the test has a warning it's not supposed to have but has been hidden by llvm-lit -s. That warning is currently fatal (it calls llvm_unreachable) because MCContext::diagnose() expects SrcMgr or InlineSrcMgr to be non-null but we have neither because our MCContext was set up for disassembling an executable so we didn't have a SourceMgr. I can work around it by removing the llvm_unreachable() in MCContext::diagnose() but that results in passing a nullptr being passed to a const SourceMgr &. I think it might need to be made a const SourceMgr * to support this case

Mar 3 2021, 11:41 PM
dsanders added a comment to rG955365524aee: [MCParser] Bring back srcmanager diagnostics in AsmParser.

Sorry for the confusion. 5de2d189e6ad was supposed to reduce but not cause more confusion.

Mar 3 2021, 11:33 PM
dsanders added a comment to rG955365524aee: [MCParser] Bring back srcmanager diagnostics in AsmParser.

Thanks for this workaround. The change also caused one of our downstream unittests to fail because errors stopped being delivered to our ASSERT_EQ code. I haven't quite got to the bottom of where our DiagHandler gets set up yet but cherry-picking this caused our unittest to pass again and now I know what I'm looking for

Mar 3 2021, 8:58 PM

Mar 2 2021

dsanders added a comment to D97812: [mir] Fix confusing MIR when MMO's value is nullptr but offset is non-zero.

Note: I went with undef in this patch because the keyword already exists but it occurred to me while posting that that could also be confusing. The location is defined, we just don't know what it is. We might want to consider something more direct like 'unknown'. Any thoughts on the spelling to use?

Mar 2 2021, 3:06 PM · Restricted Project
dsanders requested review of D97812: [mir] Fix confusing MIR when MMO's value is nullptr but offset is non-zero.
Mar 2 2021, 3:03 PM · Restricted Project

Feb 24 2021

dsanders accepted D97410: [llvm] Check availability for os_signpost.

LGTM with a couple nitpicks

Feb 24 2021, 4:05 PM · Restricted Project

Feb 12 2021

dsanders accepted D96589: [GlobalISel] Simpler verification of G_SEXT_INREG and G_ASSERT_ZEXT.

LGTM

Feb 12 2021, 11:01 AM · Restricted Project

Feb 8 2021

dsanders accepted D78796: [Support] Refactor LEB128 encoding into an input iterator.

FWIW, I still feel the encoder state shouldn't live inside the iterator but I am a bit happier that the iterator doesn't own the non-trivial encoder logic anymore. It's more consistent with pointing into a container rather than being the container, even if the pointer is unusually descriptive and the container is really more of a description of all the possible containers disambiguated by references to a specific element. I do wonder why do the logic and not the storage though. All that said, pragmatically I don't really want to insist on that as it's been in review for a very long time now so I think the middle ground is, LGTM as it currently is and if someone needs to use it in a generator-like style at some point they'll be the ones to put detail::encodeLEB128Byte() and the state in an object and have the iterators use that. Similarly, if we grow more places where iterators don't make sense they can either use the existing functions or do the same transformation just described.

Feb 8 2021, 3:47 PM · Restricted Project