Remove the irrelevant change on .clang-format.

In D87939#2285876, @qcolombet wrote:

The comment seems outdated, if my understanding is right, and even the original code cannot perform that change since, once the 2nd COPY with same source is found in L1407, the check @ L1419 just skips that earlier as the 1st COPY has no subreg and the 2nd COPY has sub1.

Good point!

Now, I am wondering why is this change not just NFC then?

In D87939#2285776, @qcolombet wrote:

Hi @hliao,

I must be missing something, but it feels to me that this patch is actually making the situation worse.

Could you look at my example inlined below and explain how it would still work with this patch?

Cheers,
-Quentin

In D87972#2284096, @lebedev.ri wrote:

In D87972#2284064, @xbolva00 wrote:

In D87972#2284060, @MaskRay wrote:

I have tested this patch internally and seen gains and losses. On one document search related benchmark 3~5% improvement. One zippy (snappy) there is 3~5% regression. Perhaps we do need a conditional extra SROA run.

Snappy - you mean public https://github.com/google/snappy?

Well, it should be possible to analyze it...

@lebedev.ri any perf data from testsuite/rawspeed?

I did look.

sroa-after-unroll.rsbench.txt68 KBDownload

This suggests that geomean is -0.8% runtime improvement,
with ups&downs.

But as i have said in the patch's description, i stumbled into this when writing new code, where the effect is much larger.

Rebase.

This patch enhances the peephole-opt to fix the redundant copy issues once to be fixed in D87556. With the enhancement, we could remove that redundant COPY locally. Test cases are revised due to the code quality improvement or change. Fortunately, AMDGPU and ARM tests need addressing that difference.

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947880, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947851, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947724, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947723, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947722, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947689, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947688, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947687, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947684, @arsenm wrote:

Can you revert this? I think that this transform itself is a workaround, and even if it were a good idea, I think it doesn't belong in another loop over the function in finalizeLowering

Could you elaborate on why that would be a workaround? Basically, after instruction selection, the COPY from SGPR to VGPR should be lowered to a native instruction.

Because this should be done after register allocation like is already done. Replacing a copy with something else should only interfere with generic optimizations

That would be too late for MachineCSE and other optimization to remove the redundant COPYs and reduce the register usage. Moving that after RA won't reduce register pressure.

So you are working around something MahcineCSE isn't doing on copies. You could just have MachineCSE do this for these copies

MachineCSE and other optimizations are designed not to handle that target-independent COPY or like, which is added with the intention that, potentially, the source and destination operands are coalesced and that COPY is removed finally. As SGPR and VGPR are different register banks and won't be coalesced anyway, native instruction should be used instead.

This does not make sense, COPY is what generic optimizations do understand. If it's useful to CSE cross bank copies, MachineCSE should handle them. As a representational choice, not-copy is worse than copy

A target-independent cross-bank COPY is definitely useful but the current COPY should not be used for that purpose considering how it's used in RA-related passes, especially only architectural constraints are changed between the source and destination operands where the propagation should be stopped.

This is exactly what copy is for. It wouldn't make sense to have a separate copy for this

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947724, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947723, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947722, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947689, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947688, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947687, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947684, @arsenm wrote:

Can you revert this? I think that this transform itself is a workaround, and even if it were a good idea, I think it doesn't belong in another loop over the function in finalizeLowering

Could you elaborate on why that would be a workaround? Basically, after instruction selection, the COPY from SGPR to VGPR should be lowered to a native instruction.

Because this should be done after register allocation like is already done. Replacing a copy with something else should only interfere with generic optimizations

That would be too late for MachineCSE and other optimization to remove the redundant COPYs and reduce the register usage. Moving that after RA won't reduce register pressure.

So you are working around something MahcineCSE isn't doing on copies. You could just have MachineCSE do this for these copies

MachineCSE and other optimizations are designed not to handle that target-independent COPY or like, which is added with the intention that, potentially, the source and destination operands are coalesced and that COPY is removed finally. As SGPR and VGPR are different register banks and won't be coalesced anyway, native instruction should be used instead.

This does not make sense, COPY is what generic optimizations do understand. If it's useful to CSE cross bank copies, MachineCSE should handle them. As a representational choice, not-copy is worse than copy

Revise formatting following the clang-format suggestion.

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947722, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947689, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947688, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947687, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947684, @arsenm wrote:

Can you revert this? I think that this transform itself is a workaround, and even if it were a good idea, I think it doesn't belong in another loop over the function in finalizeLowering

Could you elaborate on why that would be a workaround? Basically, after instruction selection, the COPY from SGPR to VGPR should be lowered to a native instruction.

Because this should be done after register allocation like is already done. Replacing a copy with something else should only interfere with generic optimizations

That would be too late for MachineCSE and other optimization to remove the redundant COPYs and reduce the register usage. Moving that after RA won't reduce register pressure.

So you are working around something MahcineCSE isn't doing on copies. You could just have MachineCSE do this for these copies

singlethread scope is added for completeness for the atomicity considering signal handling. It may not be applicable to GPU yet. We could simply ignore them in the late phases of the compiler. If desired, I may remove it as well.

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947688, @arsenm wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947687, @hliao wrote:

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947684, @arsenm wrote:

Can you revert this? I think that this transform itself is a workaround, and even if it were a good idea, I think it doesn't belong in another loop over the function in finalizeLowering

Could you elaborate on why that would be a workaround? Basically, after instruction selection, the COPY from SGPR to VGPR should be lowered to a native instruction.

Because this should be done after register allocation like is already done. Replacing a copy with something else should only interfere with generic optimizations

In rGc3492a1aa1b98c8d81b0969d52cea7681f0624c2#947684, @arsenm wrote:

Can you revert this? I think that this transform itself is a workaround, and even if it were a good idea, I think it doesn't belong in another loop over the function in finalizeLowering

LGTM

Follow suggestions from clang-tidy.

With this patch, we reduce the register pressure for certain tests which could be improved up to 10%. As the code generated would be visibly different from the previous one, regression tests are updated as well.

Basically, the logic is wrong as we only canonicalize the condition and operands in the select. But, we also need to inverse the flavor to match the original logic. The original change plus inversing flavor is committed in 41e68f7ee7b

In D87109#2255220, @arsenm wrote:

Testcase? Or cleanup a few places where I've worked around this. I assume you found another

Clean-up.

revise the test case

clean up.

Add a simple enough test case.

Add comment and revist the test case.

Check the explicit alignment if any and fix the negative test case.

Rebase.

Add the invalid case for alignment parsing.

Minor coding style fix.

Rebase

Revise following comments.

Remove adjustLDSSizeForDynLDSAlign.

Add preFinalizeLowering so that both DAGISel and GISel shares the same path
to adjust LDS size.

Add GlobalISel and MIR support.

Address the alignment issue.

Revise the comment.

In D85276#2200108, @tra wrote:

In D85276#2199655, @yaxunl wrote:

Do we need to disable pgo and coverage mapping for device compilation? Or it is already disabled?

We already disable profiling during device compilation for NVIDIA and AMD GPUs:
https://github.com/llvm/llvm-project/blob/394db2259575ef3cac8d3d37836b11eb2373c435/clang/lib/Driver/ToolChains/Clang.cpp#L4876

LGTM

In D80858#2177159, @tra wrote:

It's a good point. Perhaps this is one of the cases where we should *not* follow nvcc.
We can't have our cake (preserve static behavior) and eat it (treat it as non-static in case something on the host side may decide to use an API which uses symbol names). Something's got to give. While we could make it work in some cases, I don't think we can make it work consistently.
I think it would be reasonable to restrict APIs that access symbols by name to be applicable to visible symbols only.

In D80858#2177104, @yaxunl wrote:

In D80858#2171266, @hliao wrote:

We may try to solve the issue without RDC firstly, where we don't need to change that static variable name (if the runtime maintains the device binaries correctly.) We only need to ensure the linker won't remove their symbols.

This is essentially externalizing it in linker. However, it may not work.

Consider a static device var was accessed in host code through hipMemCpyToSymbol or hipMemCpyFromSymbol. In device code, this static var may be initialized through host code, or its final value may be read by host code after kernel execution. The existence of these operations mean that this static device variable is actually having external linkage, instead of internal linkage, since it is accessed by external modules. Fail to reflect this truth in IR will cause optimization passes to make incorrect assumptions about this variable and perform incorrect optimizations on it. e.g. the llvm passes can assume the value in this static var never changes, or its final value will not be used, then the llvm passes may simply remove it. Marking it as used will not solve the issue, since the llvm passes may still assume its value never changes after initialization, whereas in reality it may be changed by hipMemCpyToSymbol before kernel execution.

In D80858#2170821, @tra wrote:

In D80858#2170781, @hliao wrote:

The problem is not whether we have solution to tell them but when we need to add that. Not all static device variables need to be visible to the host side. Externalizing them adds the overhead for the linker and may pose additional restrictions on aggressive optimizations. Do we have to support every ambiguous usage in the burden of the compiler change?

It's a multi-part problem.
The key request here is, IIUIC, to make TU-local variables visible within the TU on both host and the device.
If and how it should be implemented is up for the discusstion.
For me the request is reasonable and it would be good to have it as it would make the code behave according to general expectations of how TU-local variables behave -- "if it's in the same source file, it should be accessible from the functions in that file".

In D80858#2170668, @tra wrote:

In D80858#2170604, @hliao wrote:

In D80858#2170547, @tra wrote:

Would it work if we generate a globally unique visible aliases for the static vars and use the alias' name to register device-side entities without changing their visibility?

We still need to define how a static device variable should be visible on the host side.

As far as the host is concerned, if a 'static' variable is within the same TU it (or, rather, its shadow) should be accessible.

How that behaves in the context of relocatable code generation as well as anonymous namespaces.

AFAICT it would just work -- each TU only sees static things within that TU on both host and device sides. Visible module-unique aliases would allow host-side binary to get address of an otherwise non-visible variables. I expect that will work with RDC -- because we didn't change the visibility of the symbols themselves, they behave according to the regular linking rules. The aliases are also globally unique, so they should present no issues either.

In D80858#2170547, @tra wrote:

Would it work if we generate a globally unique visible aliases for the static vars and use the alias' name to register device-side entities without changing their visibility?

In D80858#2170533, @yaxunl wrote:

In D80858#2170328, @hliao wrote:

In D80858#2169534, @yaxunl wrote:

Another reason is that we need to support it in rdc mode, where different TU can have static var with the same name.

That's an issue of our current RDC support through LLVM IR instead of native code. The name conflicts are introduced as we link all TUs into a single module at IR level. The frontend should not be changed to support that.

I am not sure if ISA level linking would help for this. My understanding is that the linker will rename the internal symbols having the same name from different objects. It does not matter if it is llvm-link or native linker. Once they are renamed we can no longer find them by name.

I don't see a reason why we cannot support accessing static device variable in FE. We have requests for this feature from different users.

In D80858#2169534, @yaxunl wrote:

Another reason is that we need to support it in rdc mode, where different TU can have static var with the same name.

In D80858#2169399, @yaxunl wrote:

In D80858#2169295, @hliao wrote:

I don't that's proper way to support file-scope static device variables. As we discuss APIs like cudaMemcpyToSymol, that's a runtime API instead of driver API. The later needs to specify the module (or code object) id in addition to a symbol name and won't have the conflict issues. For the runtime API, all named device variables (static or not) are identified at the host side by their host stub variables. That stub variable is used to register the corresponding device variables associated with a module id to unique identify that variables across all TUs. As long as we look up device variables using their host stub variable pointers, they are uniquely identified already. The runtime implementation needs to find the module id and the variable symbol from the pointer of its host stub variable. It's not the compiler job to fabricate name uniquely across TUs.

The problem is that even though the static variable is registered through __hipRigisterVariable, the runtime still relies on looking up symbol name to get the address of the device variable. That's why we need to externalize the static variable.

I don't that's proper way to support file-scope static device variables. As we discuss APIs like cudaMemcpyToSymol, that's a runtime API instead of driver API. The later needs to specify the module (or code object) id in addition to a symbol name and won't have the conflict issues. For the runtime API, all named device variables (static or not) are identified at the host side by their host stub variables. That stub variable is used to register the corresponding device variables associated with a module id to unique identify that variables across all TUs. As long as we look up device variables using their host stub variable pointers, they are uniquely identified already. The runtime implementation needs to find the module id and the variable symbol from the pointer of its host stub variable. It's not the compiler job to fabricate name uniquely across TUs.

Hi @rsmith, @rjmccall and @tra what's your suggestion to make progress on this review?

In D84252#2164763, @arsenm wrote:

I would expect the generic code to add this

LGTM as a quick fix.