User Details
- User Since
- Nov 14 2016, 12:37 PM (230 w, 14 h)
Thu, Apr 8
Tue, Apr 6
I'd like @pengfei to reply to this question. I think the overall idea is that many of the optimizations are pattern based, and the existing pattern wouldn't match the new intrinsic.
How is llvm.arith.fence() different from using "freeze" on a floating-point value? The goal isn't really the same, sure, but the effects seem similar at first glance.
Initially we thought the intrinsic "ssa.copy" could serve. However ssa.copy is for a different purpose and it gets optimized away. We want arith.fence to survive through codegen, that's one reason why we think a new intrinsic is needed.
Wed, Mar 31
Mon, Mar 15
Mar 11 2021
Mar 10 2021
I applied this patch to an internal workspace and did smoke testing on Linux and Windows--didn't see any problems. But I defer to others to +1.
Mar 9 2021
Mar 8 2021
I got a bug report about this patch, see https://bugs.llvm.org/show_bug.cgi?id=49479. I put a patch to fix it here, https://reviews.llvm.org/D98211
Mar 4 2021
Thanks @ABataev !
Mar 3 2021
Mar 2 2021
Thanks I'll wait to see if there are any more remarks, and push it later today or tomorrow
@aaron.ballman Is this what you have in mind to improve the diagnostic? There are other places in the file that assume FDecl can be null so I just copied that code. Also since the arm warning diagnostic is exactly similar, I added the additional information, note diagnostic, showing where the callee is declared.
Feb 25 2021
I rebased the patch and basically made no other changes. On the review there's a suggestion that 'blocklist' isn't the best word choice, do you have suggestions?
Feb 22 2021
Feb 20 2021
What do you recommend as next steps, incrementally? Is there more clarity about option names?
Feb 18 2021
Feb 12 2021
Would you use ignorelist (skiplist) in the option name as well as the filename? Can you recommend a name for the antonym as well (i.e. replacement for whitelist)
I can change D82244 used blocklist to denylist . If there is no need for compatibility, I'll just replace the strings. If there is need for compatibility, I can make blocklist an alias.
I don't think we need compatibility for D82244. If we change clang, we can update D82244 to the same for consistency.
Yes I agree.
Feb 8 2021
There is a pre-existing patch https://reviews.llvm.org/D82244 using block and allow, that's why I chose blocklist. Although going in cold I would prefer to use allow and deny, and block seems to me a little like cheating (too similar to black). However I can "disagree and commit" (cf. https://en.wikipedia.org/wiki/Disagree_and_commit) with this choice of wording. I can search out the awkward Blocklisted and come up with different phrasing in the prose, but I think names in the program should use blocklist so that they can be easily connected to the option name for future maintainers of llvm. I'll check this.
In options.td there is a way to officially mark an option deprecated by adding it to a specific group, I didn't do that yet. When could we actually eliminate the old spelling? How about December 2021?
I think we want to leave the deprecated options in for a full release cycle, so I'd say that (assuming this lands in Clang 13) we could remove support once we start work on Clang 14.
I thought it would be best to start with the external clang interface, but i also want to make more patches to eliminate whitelist and blacklist in the comments and in the object names, file names etc.
Fantastic, thank you!
Feb 6 2021
Feb 5 2021
I found a way to get the macro FLT_EVAL_METHOD set properly, so this patch is ready for your review, thanks!
Feb 4 2021
Should we add tests for mlong-double-64, -80, -128?
Feb 3 2021
Feb 2 2021
I fixed the test case like @erik.pilkington requested in 9a5dc01e4b6
Feb 1 2021
Jan 26 2021
Jan 11 2021
@rjmccall Hoping you can take a look at this patch
Dec 23 2020
Dec 14 2020
I don't have expertise in this area.
Dec 13 2020
Dec 12 2020
Thanks for the review!
Dec 10 2020
Answering my own questions
I found the place where the call was created and updated the CC
Likewise device_side_enqueue_block_invoke_9_kernel has spir_kernel CC. How does the CC get set to spir_kernel, I don't see where that is happening
The function is declared with "OpenCL Kernel" calling convention, but target-specific ClangCallConvToLLVMCallConv converts that to SPIR_KERNEL that's why I couldn't find it when doing a text search in clang
define internal spir_func void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %.block_descriptor, i8 addrspace(3)* %p1, i8 addrspace(3)* %p2, i8 addrspace(3)* %p3) #1 {
define internal spir_kernel void @__device_side_enqueue_block_invoke_9_kernel(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3) #5 {call void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
Fixed a couple CallInst, setting the CallingConvention from information in the Function node. Updated LIT tests.
Dec 9 2020
I rebased and applied clang-format patch
Dec 8 2020
Thanks @Anastasia ; I modified CGBuiltin.cpp to use EmitRuntimeCall when creating calls to runtime functions instead of Builder.CreateCall so this is setting the CallingConvention on the Call node using the ABIInfo. I changed a few lit tests that were failing because of this.
Dec 7 2020
There are many RuntimeCalls that are created in Clang, including _read_pipe, all the OpenMP runtime calls, etc. Shall they all have their calling conventions set from the ABIInfo? Currently those calls are being set to 0.
ready for review, thanks!
Thanks for the additional info @kpn
I'm going to make further changes to this patch, it's not working as desired.
Dec 6 2020
Dec 5 2020
Nov 30 2020
Nov 7 2020
I submitted the test improvements separately, i'm going to push this version, thanks all
Nov 6 2020
you're right about the regex syntax. i think the 4088 is over the max repetition count and there's also a problem with balanced. not sure what i did wrong here. if you have a chance to look at this otherwise i'll toil over the weekend, i'm off for the day--cheers. here's what i'm trying now just to see if it would work. can i nest repetition counts?
Fixed test case according to @dblaikie 's suggestion and input, thanks a lot David, the test case is certainly a lot easier to read and understand now. Anything else?
Nov 5 2020
I'm sorry, I don't see how to build up an identifier with an arbitrary number of characters using the token pasting, for example an ident with 4095 characters is not a power of 2. I tried pasting together X2048 X1024 X512 etc but that doesn't work
fu.cpp:12:18: error: pasting ")" and "X16" does not give a valid preprocessing token
#define FUN X32(x) ## X16(x)
^
fu.cpp:13:1: note: in expansion of macro ‘FUN’
I want the test case to show the transition between when md5 is not needed and when it's needed, i want to build strings that might not be power of root string. I could add more comments to the test case, perhaps, "the strlen of the Microsoft mangled name is X" for the one case, and "the strlen of ... is X+1" for the other.
Nov 4 2020
Add lit test case, based on test case in bug report
Nov 3 2020
Oct 30 2020
Oct 29 2020
respond to @aaron.ballman 's review
Oct 28 2020
The failing unit test reported above, sizes.cpp, looks like it should be an Expected Fail
Modified according to @kpn suggestion: if the command line has option fexperimental-strict-floating-point, the float pragmas will not be ignored.
Oct 27 2020
Actually kludging it by just removing the assert isn't going to work. I'll ping Pengfei to see about developing a patch for this problem.
bug report
Thank you. I don't know the solution but I can remove the assert and create a bug report to have it resolved.