LGTM

LGTM

In D116854#3229369, @yonghong-song wrote:

Without this patch, it leads to process crashes when unsupported gcc builtins are used, for example.

This is not correct description. The "llvm_unreachable" is intentional to catch all unsupported isd opcodes. If we find one unsupported isd opcode, we can add one case similar to ISD::DYNAMIC_STACKALLOC.
Please do have enough details in the commit messages so people can reproduce the issue if possible.

In D114894#3165130, @fhahn wrote:

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

In D25618#3135224, @foad wrote:

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?

LGTM

In rGe892705d74c7366a1404a3b3471001edaa7659f8#1010526, @arsenm wrote:

In rGe892705d74c7366a1404a3b3471001edaa7659f8#1010520, @dsanders wrote:

In rGe892705d74c7366a1404a3b3471001edaa7659f8#1010512, @arsenm wrote:

In rGe892705d74c7366a1404a3b3471001edaa7659f8#1010405, @kschwarz wrote:

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

In rGe892705d74c7366a1404a3b3471001edaa7659f8#1010512, @arsenm wrote:

In rGe892705d74c7366a1404a3b3471001edaa7659f8#1010405, @kschwarz wrote:

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

In rGe892705d74c7366a1404a3b3471001edaa7659f8#1010405, @kschwarz wrote:

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.

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.

LGTM, Thanks!

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

In D103352#2791465, @bogner wrote:

How much work is left to just remove this completely?

LGTM too. Thanks!

LGTM

LGTM. Sorry for the long wait

LGTM with the check in ProcessBlockPRE

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

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

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)

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.

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

LGTM

The patches I mentioned are in D100253

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

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

In rG14580ce2fdd1898d130b20d9eb21bc4281868e7c#988453, @Paul-C-Anagnostopoulos wrote:

@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 ?.

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.

Hi Paul,

LGTM

LGTM

LGTM

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

In D99692#2662702, @bogner wrote:

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"

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.)

In D74588#2660144, @aaronpuchert wrote:

In D74588#2659652, @dsanders wrote:

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.

In D74588#2659067, @aaronpuchert wrote:

In D74588#2658871, @beanz wrote:

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.

In D74588#2657461, @beanz wrote:

In D74588#2654289, @aaronpuchert wrote:

In D74588#2654185, @beanz wrote:

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.

In D99033#2645272, @aemerson wrote:

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.

LGTM

In D98046#2629692, @dsanders wrote:

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

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?

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.

LGTM