I will let others comment, but I think this is a perfectly reasonable alternative, and I much prefer using indentation over the delimiter-remark approach. LGTM, assuming nobody else objects

Thank you for the patch! I will leave it to others with more knowledge of the history of diagnostics to comment on whether the premise is acceptable, but I have a few technical comments

In D123878#3507378, @vangthao wrote:

I am not sure if allowing clang to accept newlines is a good idea. It seems like clang wants to know what type of message is being outputted. For example whether this is a remark, warning, etc. but allowing for a diagnostic to output their own newline makes it ambiguous where exactly that output is coming from.

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.

Even with newlines forced via extra remarks, I'm not a big fan of the "-----------------------" remark; it doesn't interact well with other random remarks in the output, for example when I enable all remarks using the pattern '.*' I see:

remark: foo.cl:27:0: AMDGPU DAG->DAG Pattern Instruction Selection: Function: test_kernel: MI Instruction count changed from 0 to 6; Delta: 6
remark: foo.cl:27:0: 0 stack bytes in function
remark: foo.cl:42:0: AMDGPU DAG->DAG Pattern Instruction Selection: Function: test_func: MI Instruction count changed from 0 to 4; Delta: 4
remark: foo.cl:42:0: 0 stack bytes in function
remark: foo.cl:42:0: SI insert wait instructions: Function: test_func: MI Instruction count changed from 4 to 5; Delta: 1
remark: foo.cl:8:0: AMDGPU DAG->DAG Pattern Instruction Selection: Function: empty_kernel: MI Instruction count changed from 0 to 1; Delta: 1
remark: foo.cl:8:0: 0 stack bytes in function
remark: foo.cl:52:0: AMDGPU DAG->DAG Pattern Instruction Selection: Function: empty_func: MI Instruction count changed from 0 to 1; Delta: 1
remark: foo.cl:52:0: 0 stack bytes in function
remark: foo.cl:52:0: SI insert wait instructions: Function: empty_func: MI Instruction count changed from 1 to 2; Delta: 1
remark: <unknown>:0:0: BasicBlock:
: 2

Rebase

In D121951#3414271, @sameerds wrote:

In D121951#3411856, @scott.linder wrote:

@yaxunl Does excluding device-libs via COV_None make sense?

@sameerds Would you still rather we just not add this attribute in the frontend at all? I'm OK with it if that is the consensus

Yes, I still think there is no need to emit that attribute in the frontend. It will always be inferred by the Attributor when optimization is enabled. This also eliminates the check for COV_None and there seems to be some uncertainty about COV_None anyway. This also eliminates the updates to all the tests where the no-hostcall-ptr attribute does not actually matter. If ever we need to check if hostcall is being used on OpenCL and COV < 5, we should do it per feature and inform the user appropriately.

Remove frontend changes, and just drop the diagnostic in the backend. Update
some comments and naming.

LGTM

@yaxunl Does excluding device-libs via COV_None make sense?

Do not add the no-hostcall attribute for "COV_None" modules, to allow for
the OpenCL implementation of hostcall in the device-libs.

Eliminate diagnostic, and instead add function attribute in Clang for pre-COV_5
OpenCL codegen.

In D121951#3393928, @sameerds wrote:

In D121951#3392470, @scott.linder wrote:

In D121951#3391472, @sameerds wrote:

The check for "__ockl_hostcall_internal" is not longer relevant with the new hostcall attribute. Can we simply remove this check? What is the situation where the warning is still useful?

I wasn't aware of the new attribute, I'm happy to just delete it.

Yeah, it happened in D119216.

But I am still curious about the check itself. Do we need to worry about a situation where it is important to check whether OpenCL printf (non-hostcall) and some other hostcall service are active in the same application?

In D121951#3391472, @sameerds wrote:

The check for "__ockl_hostcall_internal" is not longer relevant with the new hostcall attribute. Can we simply remove this check? What is the situation where the warning is still useful?

For reference, the error I'm changing to a warning was added as part of https://reviews.llvm.org/D70038, and the issue that made me consider this approach appears when building OCL conformance tests at -O0

In D121242#3371393, @rampitec wrote:

Scott, I appreciate your doc review. It is a lot of work. Do you mind to fix doc typos in a followup change? It is a real pain to maintain the downstream merge, it will be easier to fix when it is merged back.

I noted some small nits, but otherwise LGTM. I won't accept, as there are others who should probably review.

Thank you for following up! The change looks correct, but I have some nits about the general approach. Let me know what you think.

In D102590#3349510, @HassanElDesouky wrote:

@scott.linder I don’t have a commit permission so I’d love it if you can commit it on my behalf. Thanks!

LGTM, thank you!

LGTM, thank you!

Implement explicit return version of llvm::visit

No worries, I understand the patch is pretty big and the template code is (unfortunately) dense.

Rebase, ping

Rebase

In D104827#3140973, @scott.linder wrote:

In D104827#3138627, @dexonsmith wrote:

In D104827#3137807, @scott.linder wrote:

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.

It seems like DIArgList is very similar to LocalAsMetadata. I feel like it'd be less error-prone for it to use/inherit-from ReplaceableMetadataImpl, just like ValueAsMetadata does. Then it has the machinery to RAUW/delete itself when necessary.

(Moreover, given that it doesn't have any operands, I find it suspicious that it inherits (albeit indirectly) from MDNode. It should probably be a direct subclass of Metadata. This would mean it couldn't inherit from DINode which might complicate some debug info code; if you're going to clean that up I suggest doing it later as a separate patch.)

I looked into deriving DIArgList from ReplaceableMetadataImpl, and it does get awkward quickly that it is an MDNode. It seems like there is a fair amount of code around tracking/RAUW which relies on MDNodes being eventually "resolveable". The result is that I don't think I can inherit from ReplaceableMetadataImpl without also changing the inheritence from MDNode to Metadata as you suggest.

I'll look at what effect that has on any debug-info related code, but I do worry that no matter what there will be a fair amount of special-casing.

LGTM, with only typos and minor nits around wording.

I went ahead with just leaving DIArgList alone, let me know what you think

I'm pushing the (rough) patch which tries to make the switch from inheriting
DIArglist from MDNode to ReplaceableMetadataImpl.

In D104827#3138627, @dexonsmith wrote:

In D104827#3137807, @scott.linder wrote:

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.

It seems like DIArgList is very similar to LocalAsMetadata. I feel like it'd be less error-prone for it to use/inherit-from ReplaceableMetadataImpl, just like ValueAsMetadata does. Then it has the machinery to RAUW/delete itself when necessary.

(Moreover, given that it doesn't have any operands, I find it suspicious that it inherits (albeit indirectly) from MDNode. It should probably be a direct subclass of Metadata. This would mean it couldn't inherit from DINode which might complicate some debug info code; if you're going to clean that up I suggest doing it later as a separate patch.)

In D104827#3136642, @scott.linder wrote:

Add enforcement of the always-owned-by-MetadataAsValue property of DIArgList to
the metadata verifier. I'm not sure this is sufficient, but at least it is a
start.

Fixed the handling of the uniquify()!=this case in
DIArgList::handleChangedOperand to assert if the above property doesn't hold.

Add unittests for both changes above; the reproducers for this in IR rely on
specific sequences of optimizations, and would be much more fragile.

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

Add enforcement of the always-owned-by-MetadataAsValue property of DIArgList to
the metadata verifier. I'm not sure this is sufficient, but at least it is a
start.

In D104827#3128572, @scott.linder wrote:

Update DIArgList::handleChangedOperand to propagate in-place changes which
would require the DIArgList become distinct up to the owning MetadataAsValue.

This feels a bit odd, but so does allowing DIArgList to be distinct as an
implementation detail, while never serializing that fact (and so making a
round-trip lossy). Semantically this may be OK, but unless we have a good
reason not to I feel like finding a way to ensure these "inline" nodes are
always uniqued will make the relationship between IR/bitcode and the in-memory
representation more obvious.

Update DIArgList::handleChangedOperand to propagate in-place changes which
would require the DIArgList become distinct up to the owning MetadataAsValue.

In D104827#3119839, @aeubanks wrote:

hi, this is causing crashes

$ build/rel/bin/opt -passes='thinlto-pre-link<O2>' /tmp/a.ll -o /dev/null
opt: ../../llvm/include/llvm/IR/Metadata.def:180: void (anonymous namespace)::ModuleBitcodeWriter::writeMetadataRecords(ArrayRef<const llvm::Metadata *>, SmallVectorImpl<uint64_t> &, std::vector<
unsigned int> *, std::vector<uint64_t> *): Assertion `!N->isDistinct() && "Expected non-distinct " "DIArgList"' failed.                                                                            
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.                                                                                                              
Stack dump:                                                                                                                                                                                        
0.      Program arguments: build/rel/bin/opt -passes=thinlto-pre-link<O2> /tmp/a.ll -o /dev/null                                                                                                   
 #0 0x0000000001f37a13 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Unix/Signals.inc:565:13             
 #1 0x0000000001f3588e llvm::sys::RunSignalHandlers() /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Signals.cpp:98:18                                        
 #2 0x0000000001f37d7f SignalHandler(int) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Unix/Signals.inc:407:1                                               
 #3 0x00007f2003b8e8e0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x138e0)                                                                                                                
 #4 0x00007f2003668e71 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1                                                                                                                      
 #5 0x00007f2003652536 abort ./stdlib/abort.c:81:7                                                                                                                                                 
 #6 0x00007f200365241f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8                                                                                                                          
 #7 0x00007f200365241f _nl_load_domain ./intl/loadmsgcat.c:970:34                                                                                                                                  
 #8 0x00007f20036617f2 (/lib/x86_64-linux-gnu/libc.so.6+0x357f2)                                                                                                                                   
 #9 0x000000000168e506 (anonymous namespace)::ModuleBitcodeWriter::writeMetadataRecords(llvm::ArrayRef<llvm::Metadata const*>, llvm::SmallVectorImpl<unsigned long>&, std::vector<unsigned int, std::allocator<unsigned int> >*, std::vector<unsigned long, std::allocator<unsigned long> >*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/include/llvm/IR/Metadata.def:0:0
#10 0x000000000167df15 writeFunctionMetadata /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2303:3
#11 0x000000000167df15 writeFunction /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3351:3
#12 0x000000000167df15 (anonymous namespace)::ModuleBitcodeWriter::write() /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4448:7
#13 0x0000000001677d32 ~ModuleBitcodeWriterBase /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:155:7
#14 0x0000000001677d32 llvm::BitcodeWriter::writeModule(llvm::Module const&, bool, llvm::ModuleSummaryIndex const*, bool, std::array<unsigned int, 5ul>*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4626:1
#15 0x00000000016813cb llvm::WriteBitcodeToFile(llvm::Module const&, llvm::raw_ostream&, bool, llvm::ModuleSummaryIndex const*, bool, std::array<unsigned int, 5ul>*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4653:10

a.ll60 KBDownload

(I tried reducing as much as possible but there's still a lot of debug info)

In D100672#3119835, @dblaikie wrote:

In D100672#3119829, @scott.linder wrote:

Ping, @dblaikie are you OK with the patch as-is? I think it is likely not worth the effort of tracking down what MSVC configurations will accept the constexpr version.

Fair enough. I really don't like to workaround unknown issues (ends up with a form of "fear based programming" - "let's not write code that looks like that because there are issues /somewhere/ in there" - haunted graveyards ( https://www.usenix.org/sites/default/files/conference/protected-files/srecon17americas_slides_reese.pdf ) , etc) - so I think there's significant value in figuring out exactly what the limitations are/what's being worked around - but I won't insist on it.

Ping, @dblaikie are you OK with the patch as-is? I think it is likely not worth the effort of tracking down what MSVC configurations will accept the constexpr version.

I believe the only pending issue is resolved, so I plan to land this tomorrow
if nobody has any objections.

Remove misleading comment headers, strip "CSR_" prefix from regmask identifiers.

Is it possible to use soft links instead of copies? It would still be good to clean up afterwards, but if the host won't allow that cleanup in some cases at least we aren't leaving a big file around.

LGTM thank you!

In D102590#2971046, @HassanElDesouky wrote:

@scott.linder Thank you so much for the review!

1. I made the changes you pointed out regarding defining a new scanBlockScalarIndicators method.
2. I think the idea of enum BlockStyle and enum BlockChomping is really good. I will definitely implement it but maybe in another PR/diff. -- if that's okay :)

So, I tried to work out the behavior of the spec parser, and I think my understanding of empty is faulty. Compare http://ben-kiki.org/ypaste/data/26953/index.html (all content is non-space characters) with http://ben-kiki.org/ypaste/data/26954/index.html (the middle line content is a single space).

I'm not sure I'm the best person for the job, but I tried to give my thoughts.

Rebase

Eliminate the getDistinct method entirely from DIExpression and DIArgList. I
went about this by splitting up the DEFINE_MDNODE_* macros so each MDNode
class can use an appropriate version based on their distinct/uniqued
properties.

LGTM, thank you for catching these!

LGTM, as long as @MaskRay is satisfied with the changes around easy_install. If MacOS still uses it but we want to discourage it on Linux then I like the current wording.

I think the change itself LGTM, but I did a grep for GNUInstallDirs and it it only used in two other places currently. I suppose that means it is OK to use, but it does seem to imply there can be cases where we install to inconsistent locations. I don't know who to add to get more info on this, though?

LGTM, I see no reason why this shouldn't apply for all generators.