Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
740 ↗ | (On Diff #70584) | I don't think this is correct when rotating by zero. That will cause us to shift right by ArgTypeSize, which will result in an undefined value: http://llvm.org/docs/LangRef.html#lshr-instruction I think we need to generate a select or conditional branch as in the original code. |
test/CodeGen/ms-intrinsics-rotations.c | ||
138 ↗ | (On Diff #70584) | Nice, testing both LP64 and LLP64. :) |
You should locally verify that this generates the correct assembly when optimizations are enabled, and if it doesn't, try to make the input look more like llvm/test/CodeGen/X86/rotate.ll
test/CodeGen/ms-intrinsics-rotations.c | ||
---|---|---|
2 ↗ | (On Diff #70584) | Can we run without -Oz? The test should still pass. |
Fix undefined value problem when rotating by zero bits
Change tests to work without -Oz flag
Fix types of arguments in tests
Yeah, I checked that it's optimized to ROL/ROR instruction - now, after looking closer, I can see that it's optimized for all functions except for the ones operating on 16-bit integers. Could that be a calculated decision, or should I try to make it generate ROL/ROR whenever it can?
Edit: I tried to make the code as similar to the one from rotate.ll as possible, and it's still not optimized to ROL/ROR for 16-bit integers, so I guess it should stay that way.
lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
740 ↗ | (On Diff #70614) | Ah, I am sure I saw that bug, but apparently I forgot about it, as on my machine it returned correct values. Thanks! |
I wouldn't worry about this too much... It is up to the backend to do the right thing.
lib/AST/ExprConstant.cpp | ||
---|---|---|
7024–7050 ↗ | (On Diff #70646) | Any reason why we need this? Given: #include <intrin.h> constexpr int x = _rotl8(1, 2); MSVC 2015 reports: error C2131: expression did not evaluate to a constant |
lib/AST/ExprConstant.cpp | ||
---|---|---|
7024–7050 ↗ | (On Diff #70646) | Hm, I don't know. Is there any reason why we shouldn't do this? I mean, I just had the feeling that if we can evaluate something during compilation time, we should to it. |
lib/AST/ExprConstant.cpp | ||
---|---|---|
7024–7050 ↗ | (On Diff #70646) | The best reason I can think of is that it allows people to use a Microsoft-specific intrinsic in ways which won't compile with Microsoft's compiler. |
lib/AST/ExprConstant.cpp | ||
---|---|---|
7024–7050 ↗ | (On Diff #70646) | Yeah, I agree, we should drop the constexpr part of this. The use case for these intrinsics is really to get at the x86 instructions. |