- User Since
- Aug 19 2013, 3:30 PM (409 w, 7 h)
Tue, Jun 15
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.
Fri, Jun 4
Wed, Jun 2
LGTM (we should wait for Matt to confirm he's ok with dealing with the high-level issue later though)
Tue, Jun 1
Fri, May 28
May 19 2021
LGTM too. Thanks!
May 5 2021
Apr 29 2021
LGTM. Sorry for the long wait
LGTM with the check in ProcessBlockPRE
Apr 26 2021
Apr 23 2021
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 22 2021
I'm not aware of anything still using it either. LGTM
Apr 19 2021
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
Patch by Roman Tereshin (it seems arcanist loses that info for the review)
Apr 13 2021
Apr 12 2021
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 11 2021
Yep, ./bin/llvm-lit -svv ../llvm-project/llvm/test/TableGen and ninja check passes
Apr 10 2021
The patches I mentioned are in D100253
I'll post a patch for the Rec10.Zero fix in a moment
Apr 9 2021
Ah, okay, that sounds reasonable. Now what is the circumstance under which ? is coerced to 0?
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 8 2021
Apr 1 2021
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
Mar 31 2021
Mar 30 2021
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 29 2021
Mar 23 2021
Mar 16 2021
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 10 2021
Mar 5 2021
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 4 2021
Mar 3 2021
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
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 2 2021
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?
Feb 24 2021
LGTM with a couple nitpicks
Feb 12 2021
Feb 8 2021
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 5 2021
I think lifting the if (DstTy.isVector()) without testing all the cases it affects is a bit risky. LGTM if that's confirmed safe or if the check is sunk to the cases you didn't mean to affect
Jan 20 2021
Jan 19 2021
LGTM with the changingInstr() to match the changedInstr() on the setReg() path.
Jan 14 2021
A sys::SmartMutex equivalent of this would LGTM. Let me know if there's a reason we should stick to std::mutex
Jan 11 2021
Mostly LGTM but I'd rather move the vector_extract back to SelectionDAGCompat.td as having it there doesn't break anything.
Jan 7 2021
Jan 6 2021
Dec 16 2020
Dec 15 2020
But since everyone else have had to do this for other targets, I suppose I will be forced to do the same.
Dec 9 2020
Dec 7 2020
Dec 2 2020
LGTM with @gjain's suggestions. I think SIRegisterInfo.cpp could also propagate MCRegister a bit further if you wanted but I didn't check all the users of SubReg
Oct 16 2020
Peng asked me to commit this off-list