User Details
- User Since
- Aug 19 2013, 3:30 PM (500 w, 5 d)
Mon, Mar 13
Fri, Mar 10
Wed, Mar 1
LGTM
Jan 12 2023
I don't have time for a proper review at the moment as I'm still dealing with a lot of downstream problems but I have a few comments
Nov 14 2022
LGTM
Oct 25 2022
LGTM, ideally as you say this ought to be part of the partitioning so the generated can code efficiently get to the relevant leaves more directly but this is fine for now
Oct 17 2022
While the transform implemented here is correct, it is part of the wrong pass. This should be part of InstSimplify in simplifyExtractElementInst(). Can you please move it there?
Oct 10 2022
Apr 5 2022
LGTM
Mar 31 2022
LGTM
Mar 30 2022
Mar 28 2022
Jan 10 2022
Jan 4 2022
Dec 1 2021
Nov 30 2021
Nov 16 2021
Oct 14 2021
Oct 13 2021
Oct 4 2021
Sep 29 2021
LGTM
Sep 15 2021
Jul 9 2021
Jun 15 2021
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 4 2021
LGTM, Thanks!
Jun 2 2021
LGTM (we should wait for Matt to confirm he's ok with dealing with the high-level issue later though)
Jun 1 2021
May 28 2021
May 19 2021
LGTM too. Thanks!
May 5 2021
LGTM
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
Hi Paul,
LGTM
LGTM
LGTM
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
LGTM
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.)