lg

This makes sense to me.

This is an obvious fix for a use-after-free. Looks good.

I think that's a fair point for bringing it back for now. It's not supported though and we will get rid of it eventually.

The performance difference on preprocessing huge files was tiny back then, doesn't surprise me that it disappeared. What did you test this on?

I'd be interested in hearing how this is used. I added this feature as an experiment a while back but it simply didn't work as I envisioned it to. Some checks do work but the overall latency makes it unusable in an IDE setting. People repeatedly asked me to remove it because it slows down builds while not adding value.

I like it.

Thanks. There's a bit of unused code that passes isPodLike through the layers, but this is the minimal fix. Looks good to me.

Thanks! This looks good to me, but I'd like someone from sanitizer team review it too.

• Add a comment to the test.

In D54113#1287835, @kcc wrote:

Is a test possible here?

Add test

lg

lg

lg

lg

lg

I like it

meh

In D51042#1207769, @tra wrote:

This is a surprise. PTX ISA does not mention that .ftz is not applicable to cvt.*.f16.* instructions.
Is it only cvt that does not support .ftz or does it impact other instructions? PTX spec has add/sub/mul/fma/set/setp instructions that support f16 and have .ftz variant.

lg

Test case?

lg, thanks

lg

I don't know much about cuda, but this looks fine to me.

lg

Thanks, this is only the 2nd time this has happened to me. Will fix :)

cool

lg

This is fine, it's just that listing all of the instructions gives me a really bad feeling.

sad

In rC335330#256851, @stella.stamenova wrote:

@bkramer, this breaks the tests when the path to python contains spaces (which is why in the original change %python is surrounded by quotes). For example:

Command Output (stdout):
 --
$ ":" "RUN: at line 1"
$ "rm" "-f" "C:\agent1\_work\2\b\LLVMBuild\tools\clang\test\Preprocessor\Output\headermap-rel2.c.tmp.hmap"
$ ":" "RUN: at line 2"
$ "C:/Program" "Files" "(x86)/Microsoft" "Visual" "Studio/Shared/Python36_64/python.exe" "C:/agent1/_work/2/b/LLVMBuild/Release/bin\hmaptool" "write" "C:\agent1\_work\2\s\llvm\tools\clang\test\Preprocessor/Inputs/headermap-rel2/project-headers.hmap.json" "C:\agent1\_work\2\b\LLVMBuild\tools\clang\test\Preprocessor\Output\headermap-rel2.c.tmp.hmap"
# command stderr:
'C:/Program': command not found

You probably should add quotes around the python path in lit.cfg.py when you do the substitution. I can look into this on Monday if you don't have time to fix it beforehand.

Address comments.

In D48392#1138246, @brzycki wrote:

I always prefer deterministic execution (all other things being equal). Have you done any testing of performance impact to this change? The adding and erasing of Dominator elements may cost more with how the SmallVector type performs the operations.

Otherwise I think the patch looks good. However, @kuhar is the Dominator expert here and I'd recommend waiting for him to speak up before committing.

I didn't manage to get the 240 MB IR file reduced enough, triggering this bug requires a lot of jump threading, so landing this without a test case.

Did you try bugpoint? a 240MB IR fire is indeed huge but maybe letting it run over night can help reduce the problem space. Non-deterministic errors like these are very difficult to narrow down though.

I think this is fine to go in. It's not the most pretty API, but not really more unsafe than what's already there.

I don't know much about the python bindings, but this is probably fine.

lg

why?

lg

lg

This looks overly specific. Can you teach GetStringLength about this instead? All of the transformations should already work if GetStringLength returns a length of zero.

lg

Looks good. Please watch the windows buildbots carefully after landing this.

Add a FIXME for using the demanded mask of the udiv result.

lg

Would it be possible to get rid of the flag entirely and make this controlled only by the data layout? That's defined on the module level.

lgtm

lgtm

Extend getNumberOfRegisters comment.

In D46011#1076733, @arsenm wrote:

Would it be difficult to preserve the invariant if both loads are?

Improve test by checking that the merged store is not marked as invariant.

doing it later is fine with me.

Thanks! I wonder if we can come up with a better name than ObjectMemoryBuffer though. Maybe SmallVectorMemoryBuffer?

lg

lg

this is trivially fine.

this is trivially fine.

lg

lg, thanks!

I wouldn't say that this is more maintainable, but I'm not the maintainer of clang-format.

I'd like to, but I don't know when I find time to rebase this thing after more than a year of waiting for review.

In D44175#1029118, @dblaikie wrote:

What we probably need is some kind of adl_begin/adl_end functions that wrap up this functionality for easy reuse throughout our STL helpers/wrapper/etc. Though I haven't given a lot of thought to how easy/hard that'd be to implement :/

lg