myatsina added a comment.
In https://reviews.llvm.org/D15075#940711, @dolson wrote:
> Hello,
>
> In the process of upgrading from clang 3.6.1 to a newer version, I ran into
> this new error and thus imported the new intrinsics from intrin.h for rep
> movsb and friends. I see several discussion
On Thu, Nov 30, 2017 at 10:21 AM, Dan Olson via Phabricator <
revi...@reviews.llvm.org> wrote:
> dolson added a comment.
>
> Hello,
>
> In the process of upgrading from clang 3.6.1 to a newer version, I ran
> into this new error and thus imported the new intrinsics from intrin.h for
> rep movsb an
dolson added a comment.
Hello,
In the process of upgrading from clang 3.6.1 to a newer version, I ran into
this new error and thus imported the new intrinsics from intrin.h for rep movsb
and friends. I see several discussions in this thread about how having the
registers solely in the inputs
myatsina added a comment.
Vitaly,
Thanks for fixing the test!
Test was fixed by in
https://reviews.llvm.org/rL290621: [asan] Fix test broken by r290540
Review:
https://reviews.llvm.org/D28128
Repository:
rL LLVM
https://reviews.llvm.org/D15075
___
Both work:
#define DECLARE_ASM_REP_MOVS(Type, Movs)
\
template <> void asm_rep_movs(Type * dst, Type * src, size_t size)
{ \
__asm__("rep " Movs " \n\t"
\
: "+D"(dst), "+S"(src), "+c"(size) \
: \
: "memory"); \
}
ahatanak added a comment.
I guess it doesn't build because output constraints need "=" (e.g., "=D")?
Also, I think all registers ("D", "S", and "c") should be in both the output
and input operands list. You can probably declare new variables and use them in
the output operands (e.g., "=D"(newD
myatsina added a comment.
In https://reviews.llvm.org/D15075#631316, @ahatanak wrote:
> In https://reviews.llvm.org/D15075#631237, @myatsina wrote:
>
> > In https://reviews.llvm.org/D15075#631207, @vitalybuka wrote:
> >
> > > These patches break asan tests:
> > > http://lab.llvm.org:8011/builder
ahatanak added a comment.
In https://reviews.llvm.org/D15075#631237, @myatsina wrote:
> In https://reviews.llvm.org/D15075#631207, @vitalybuka wrote:
>
> > These patches break asan tests:
> > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/528/steps/check-asan%20in%20gcc%20build/
myatsina added a comment.
In https://reviews.llvm.org/D15075#631207, @vitalybuka wrote:
> These patches break asan tests:
> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/528/steps/check-asan%20in%20gcc%20build/logs/stdio
Vitaly ,
This patch is a gcc compatibility issue and i
vitalybuka added a comment.
These patches break asan tests:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/528/steps/check-asan%20in%20gcc%20build/logs/stdio
Repository:
rL LLVM
https://reviews.llvm.org/D15075
___
cfe-commits ma
This revision was automatically updated to reflect the committed changes.
Closed by commit rL290539: [inline-asm]No error for conflict between
inputs\outputs and clobber list (authored by myatsina).
Changed prior to commit:
https://reviews.llvm.org/D15075?vs=74500&id=82495#toc
Repository:
rL
zizhar updated this revision to Diff 74500.
zizhar added a comment.
changed :)
Thanks for the review :>
https://reviews.llvm.org/D15075
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
lib/Basic/TargetInfo.cpp
lib/Basic/Targets.cpp
lib/Headers/intrin.
ahatanak accepted this revision.
ahatanak added a comment.
LGTM with a few nits.
Please fix lib/Headers/intrin.h too and commit it as a separate patch.
Comment at: lib/Sema/SemaStmtAsm.cpp:142
+// Extracting the register name from the Expression value,
+// if there is no regis
zizhar updated this revision to Diff 74068.
zizhar added a comment.
I hope this last update finishes it,
Thanks for the review :)
https://reviews.llvm.org/D15075
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
lib/Basic/TargetInfo.cpp
lib/Basic/Targets
ahatanak added a comment.
I have a couple of minor comments, but patch looks good to me.
Comment at: lib/Sema/SemaStmtAsm.cpp:141
+// Extracting the register name from the Expression value,
+// if there is no register name to extract, returns ""
Please remove
zizhar updated this revision to Diff 73760.
zizhar added a comment.
fixed :)
https://reviews.llvm.org/D15075
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
lib/Basic/TargetInfo.cpp
lib/Basic/Targets.cpp
lib/Headers/intrin.h
lib/Sema/SemaStmtAsm.cp
ahatanak added inline comments.
> TargetInfo.cpp:430
>if (AN == Name && ARN.RegNum < Names.size())
> -return Name;
> +if (ReturnCanonical)
> + return Names[ARN.RegNum];
Please use braces or a ternary operator here.
> ahatanak wrote in Targets.cpp:2718
> Can we s
rnk added a comment.
You should use git-clang-format or some equivalent to format your change.
> TargetInfo.h:597
> + StringRef getNormalizedGCCRegisterName(StringRef Name,
> +bool ReturnCanonical = false) const;
>
format
> TargetInfo.h:600
> + virtual StringRef getConstraintRegister(
zizhar updated this revision to Diff 73617.
zizhar added a comment.
Changed the '^' to point to the clobber which conflicts.
Changed the relevant comments.
However, regarding the point about the non-alphabetical characters - It's a
different behavior for these specific characters,
since these sho
ahatanak added a comment.
This patch doesn't detect more complex conflicts (like the following code), but
I think it's OK for now.
asm ("foo" : "=Q" (x) : : "%rax", "%rbx", "%rcx", "%rdx"); // Q: Any register
accessible as rh: a, b, c, and d.
> TargetInfo.h:593
>///
>/// For examp
zizhar added a comment.
Hello :)
I will prepare another patch as you suggested,
is this patch good?
Can it be submitted?
Ziv
https://reviews.llvm.org/D15075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mai
ahatanak added a comment.
You can create a separate patch for the changes made to lib/Headers/intrin.h
and have it reviewed before committing this patch.
Also, __dst, __x and __n should be added to the output list and "memory" to the
clobber list as majnemer pointed out. I think you can use con
zizhar updated this revision to Diff 70660.
zizhar added a comment.
Hi,
I have moved the relevant code to TargetInfo as previously discussed, and
created a default implementation and the x86 implementation.
Regarding the intrinsics, I believe it should be a separate fix, as it is not
related to
ahatanak added a comment.
In https://reviews.llvm.org/D15075#486986, @zizhar wrote:
> Akira,
> You've mentioned a good point, this X86 logic should indeed be moved to
> X86TargetInfo.
> The current convertConstraint() implementation is not doing what I need – it
> doesn’t handle all the input
majnemer added a subscriber: majnemer.
Comment at: lib/Headers/Intrin.h:841-874
@@ -840,44 +840,36 @@
#if defined(__i386__) || defined(__x86_64__)
static __inline__ void __DEFAULT_FN_ATTRS
__movsb(unsigned char *__dst, unsigned char const *__src, size_t __n) {
- __asm__("rep m
zizhar added a comment.
Akira,
You've mentioned a good point, this X86 logic should indeed be moved to
X86TargetInfo.
The current convertConstraint() implementation is not doing what I need – it
doesn’t handle all the input/output constraints I need, and it returns the
constraint in a different
zizhar added a comment.
Hello Hans,
Regarding the changes in intrin.h,
You're the last to change/add these lines, I found out that there is a conflict
between the clobber list and input or output lists.
Can you review this change?
Thanks,
Ziv Izhar
https://reviews.llvm.org/D15075
__
ahatanak added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:585
@@ +584,3 @@
+ StringRef
+ TargetInfo::getNormalizedGCCRegisterName(StringRef Name,
+ bool ReturnCannonical = false)
const;
This gives bui
zizhar updated this revision to Diff 60996.
zizhar added a comment.
A redo, investigated a bit more, had to fix some things, moved it to
ActOnGCCAsmStmt.
fixed intrin.h (the patch reveals a problem in it).
added tests to check for regression and examples.
http://reviews.llvm.org/D15075
Files:
ahatanak added a comment.
> After going a bit through the log, I think that there is no reason for clang
> to not detect it, probably the check was just forgotten.
>
> This patch is the check.
Which part of your patch (in SemaStmtAsm.cpp) is supposed to catch that? I
applied this patch and
zizhar added a comment.
You understood corectly :)
After going a bit through the log, I think that there is no reason for clang to
not detect it, probably the check was just forgotten.
This patch is the check.
"r" constraint means (taken from the spec:) A register operand is allowed
provided
31 matches
Mail list logo
