This isn’t the change I expected but should work for now. I’m working on the machine equivalent of this reduction now and may make additional changes here

In D87936#3525578, @arsenm wrote:

I think addressing modes are less of a factor given that g_ptr_add is separate

I think addressing modes are less of a factor given that g_ptr_add is separate

Why can't you just mark these instructions as being terminators? AMDGPU has to glue a lot of ALU instructions to terminators, and we just define terminator and nonterminator pseudo variants

I do think the add -> or has some value, but it should be separate with the helper function

In D124044#3523534, @MatzeB wrote:

As far as FrameSetup/FrameDestroy are concerned, it's a bug if the targets break on them.

You can probably debate that with target authors :)... Anyway I don't have strong feelings and this seems valuable to move forward with.

If there's some required property, it would need to be checked by the verifier. If it's not checked by the verifier, you can't rely on it. Cases that fail the verifier are filtered out

ping

If you blindly scan forward, you're going to run into blocks that have phis that are now missing an input for the new predecessor

LGTM, this kind of fix doesn't really need review

Can you avoid adding a second run by moving the current run? The ad-hoc physreg liveness tracking after allocation is really expensive

Could you use hasExtraSrcRegAllocReq/hasExtraDefRegAllocReq?

In D123943#3470045, @qiongsiwu1 wrote:

In D123943#3465282, @arsenm wrote:

In D123943#3465190, @qiongsiwu1 wrote:

Hi Matt @arsenm !

May I check with you why commit https://reviews.llvm.org/rG9209a519180b478f7a77d7c4781ea857536d77ed does not have a link to this patch?

Thanks!!

FYI @zibi

I don't use arc which adds those comments

Ah got it! Thanks so much for getting back to me! I am a new contributor so I am trying to understand the convention from the community. If you don't mind, could you help me understand if I myself should or should not use arc? For commits I am working myself, should I link the reviews to them? Could you offer some pointers for me to stare at? I have gone through https://llvm.org/docs/Contributing.html#how-to-submit-a-patch and https://llvm.org/docs/CodeReview.html and it is not clear to me what convention I should follow.

Thanks for your patience!

In D123508#3489330, @reames wrote:

At a high level, I do very much think having atomic loads simply be normal loads with some atomic flags makes a lot of sense.

I don't particularly care about splitting the patch. You can't really break this down very far, and the individual changes aren't really individually testable

In D125212#3518566, @abinavpp wrote:

I agree with you on omitting the predicates if the field is set to
false/uninitialized. In that case, how about we add the following to
PatFrags:

// If set to true, a predicate is added that checks for at least one use of
// the return value; Otherwise no predicate is added.
bit HasUse = ?;
// If set to true, a predicate is added that checks for no use of the return
// value; Otherwise no predicate is added.
//
// The TableGen backend asserts that both HasUse and HasNoUse is not set to
// true.
bit HasNoUse = ?;

In D124844#3488068, @nhaehnle wrote:

I do plan to look into handling zero-/sign-extended sources soon. My current thinking is to make GISelKnownBits available during legalization. Are there any concerns with that / does anybody have a better idea?

In D125212#3510705, @abinavpp wrote:

In D125212#3500234, @arsenm wrote:

I do think there should be a builtin hasOneUse pattern predicate the emitter could use to verify

If we have a bit HasNoUse builtin predicate in PatFrags we can get the matcher
table to check the empty usage of the result without the generic C++ predicates.
The problem I'm seeing here is that the default behaviour should be to not check
for the use count since most of the selection doesn't care about it. Having a
ternary HasNoUse builtin is fine since we could set it to false for selection
that needs at least 1 use (e.g.: AMDGPU's return atomic ops), true for selection
that needs no use (e.g.: AMDGPU's no return atomic ops), "don't care" for the
majority of the other selections. I don't think we can rely on the '?'
(uninitialized) value since TreePredicateFn::isPredefinedPredicateEqualTo()
return false for uninitialized fields.

How should we do this?

In D125404#3517294, @andreytr wrote:

Removed more metadata mentioned by @zuban32 and removed dynamic-memmove.ll and experimental.noalias.scope.decl.ll tests mentioned by @arsenm .

Typo amdgc. in description

I thought we codegened these already? Is this missing a codegen change to use the offsets?

I don't understand the problem this is supposed to solve

I feel like I fixed this one before

In D123878#3506500, @afanfa wrote:

If possible, I would like to keep some kind of delimiter. I like the idea of having it at the beginning and at the end of the section. The best option would be to convince clang to print new lines.

But it’s not a section and no actual grouping concept here. You just happen to see this printed in order. Any delimiter should be introduced as a display function, not emitted as part of the remarks themselves

Missing test for intrinsics selection?

I do think there should be a builtin hasOneUse pattern predicate the emitter could use to verify

In D125126#3498084, @rampitec wrote:

A potentially better alternative is to use gfx940 names with _LDS_ in the mnemonic instead of a modifier. This is logically a different opcode anyway. The only downside it is not compatible with the documentation and sp3. But then it was not implemented before and therefore not used, so there shall be no compatibility problem on practice. Well, it will also be different from MUBUF. Given the difference in both semantics and addressing mode I personally would prefer it to be different opcodes. At a pseudo level it is certainly easier to have separate ops for this.

Preferences?

Also could use a test where it makes a difference

I have to look closer but my main concern is about adding an SGPR argument for this. It doesn’t correspond to a real kernel input, and we didn’t add this to the new ABI register layout proposal. What happened to using some kind of relocation for the kernel ID?

I think I've seen this one before

In D124884#3490731, @nhaehnle wrote:

What's wrong with the idea of an i32 imm %size argument? That seems to me more in line with the philosophy of caring less about types.

In D124884#3490643, @arsenm wrote:

In D124884#3489967, @rampitec wrote:

To confirm: is that OK to add yet another imm to the end of operands of the intrinsic to select a byte size? And then remove the overload. If yes I will do it tomorrow.

There's the elementtype attribute for this case which some arm intrinsics seem to be using. Not sure how you're supposed to define an intrinsic to use it though