[PATCH] D152433: [ARM,AArch64] Add a full set of -mtp= options.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a subscriber: olista01.
rengolin added inline comments.



Comment at: clang/include/clang/Driver/Options.td:3525
+   "For AArch32: 'soft' uses a function call, or 'tpidrurw', 
'tpidruro' or 'tpidrprw' use the three CP15 registers. 'cp15' is an alias for 
'tpidruro'. "
+   "For AArch64: 'tpidr_el0', 'tpidr_el1', 'tpidr_el2', 'tpidr_el3' or 
'tpidrro_el0' use the five system registers. 'elN' is an alias for 
'tpidr_elN'.">;
 def mpure_code : Flag<["-"], "mpure-code">, Alias; // Alias for 
GCC compatibility

simon_tatham wrote:
> rengolin wrote:
> > From your comment:
> > 
> > > "In AArch32, on the other hand, the _only_ thread register you can choose 
> > > (apart from 'none, use a function call') is the one that's read-only at 
> > > EL0."
> > 
> > I inferred the current alias `el0` would map to the read-only version 
> > `tpidrro_el0`.
> > 
> > Looking at the implementation below (`AArch64ExpandPseudoInsts.cpp`), `EL0` 
> > seems to be the default when choosing the thread pointer?
> If you inferred that, then I was unclear, and should reword :-)
> 
> `el0` is a name only accepted on AArch64, and maps to the AArch64 register 
> `tpidr_el0`.
> 
> The only hardware option in AArch32 (before this commit) is called `cp15` 
> (unhelpfully, since all three regs are in CP15), and is an alias for 
> `tpidruro`, which is the AArch32 register that's readonly at EL0 and writable 
> at EL1 (and in fact aliases the bottom 32 bits of tpidrro_el0).
> 
> Yes, the current defaults are different between AArch32 and 64 
> (unsurprisingly, since no register is currently supported on both), but that 
> makes sense, since Linux also seems to do things differently. On AArch32 the 
> code generation uses TPIDRURO, which unprivileged code can read but not 
> write, and on AArch64 it uses TPIDR_EL0 which unprivileged code can overwrite 
> if it wants to. I don't know why the defaults are different, but I have no 
> plan to change them here!
Right! The confusion was on my side on the overloaded `EL0` for both the 
register name in AArch64 and the processor state on both. I should have noticed 
the capitalisation difference. Now it makes sense, thanks!

> I don't know why the defaults are different, but I have no plan to change 
> them here!

Ack. First introduce functionality, then investigate why (on a separate patch).

@olista01 might remember something...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152433/new/

https://reviews.llvm.org/D152433

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152433: [ARM,AArch64] Add a full set of -mtp= options.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

The only minor visible difference is the removal of `read-tp-hard` option from 
the LLVM side, which could be used by other downstream implementations.

I personally don't think this is a big deal. First, we don't promise stability 
on that layer, and second, it would be trivial to find out what the option has 
changed to.

This looks good to me, perhaps clearing the potential confusion on the commit 
message (inline comment).

Give it some time for other people to see it.

Thanks!
Renato




Comment at: clang/include/clang/Driver/Options.td:3525
+   "For AArch32: 'soft' uses a function call, or 'tpidrurw', 
'tpidruro' or 'tpidrprw' use the three CP15 registers. 'cp15' is an alias for 
'tpidruro'. "
+   "For AArch64: 'tpidr_el0', 'tpidr_el1', 'tpidr_el2', 'tpidr_el3' or 
'tpidrro_el0' use the five system registers. 'elN' is an alias for 
'tpidr_elN'.">;
 def mpure_code : Flag<["-"], "mpure-code">, Alias; // Alias for 
GCC compatibility

From your comment:

> "In AArch32, on the other hand, the _only_ thread register you can choose 
> (apart from 'none, use a function call') is the one that's read-only at EL0."

I inferred the current alias `el0` would map to the read-only version 
`tpidrro_el0`.

Looking at the implementation below (`AArch64ExpandPseudoInsts.cpp`), `EL0` 
seems to be the default when choosing the thread pointer?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152433/new/

https://reviews.llvm.org/D152433

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149946: [LoongArch] Define `ual` feature and override `allowsMisalignedMemoryAccesses`

[Renato Golin via Phabricator via cfe-commits]

rengolin added a reviewer: peter.smith.
rengolin added a comment.

Rationale and implementation make sense to me. I'll let this one sit so that 
other folks, including Arm, can have a look, too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149946/new/

https://reviews.llvm.org/D149946

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145833: Switch ABI references to env/environment

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

IIRC, "abi" used to be what Arm called, but "env" is equally good. And it it's 
consistent with `Triple.h`, even better. LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145833/new/

https://reviews.llvm.org/D145833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141750: [docs] Add llvm & clang release notes for LoongArch

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Nice! Thanks for the detailed reporting. LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141750/new/

https://reviews.llvm.org/D141750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134638: [Clang][LoongArch] Add inline asm support for constraints k/m/ZB/ZC

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.

Looks good, with some nits. Thanks!




Comment at: llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp:76
+  // TODO: handle extra code.
+  if (!ExtraCode) {
+const MachineOperand  = MI->getOperand(OpNo);

NIT: Use early exit.

if (ExtraCode)
  return false;



Comment at: llvm/test/CodeGen/LoongArch/inline-asm-constraint-ZC.ll:16
+;
+; LA64-LABEL: ZC_offset_neg_32769:
+; LA64:   # %bb.0:

Same comment as before, these CHECK lines look identical for both targets...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134638/new/

https://reviews.llvm.org/D134638

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134157: [Clang][LoongArch] Add inline asm support for constraints f/l/I/K

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Thanks, looks good to me!

I wouldn't split this in two commits. One of the benefits of having a monorepo 
is that we don't have to do that anymore. :)




Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:2064
+uint64_t CVal = C->getSExtValue();
+if (isInt<16>(CVal))
+  Ops.push_back(

SixWeining wrote:
> rengolin wrote:
> > Shouldn't this break if the constant isn't in the right type? What happens 
> > if it isn't? 
> > 
> > It seems it just doesn't append the operand and return. Wouldn't that just 
> > break the op's format?
> > 
> > On Arm, the logic is simpler:
> >  - Iterate through all constraints, validating the input
> >  - If valie, set the Result and append to the Op at the end
> >  - Otherwise bail and let `TargetLowering::LowerAsmOperandForConstraint` 
> > handle it.
> If the constant isn't in the right type, SelectionDAGBuilder will emit an 
> error. There is a test 
> `llvm/test/CodeGen/LoongArch/inline-asm-constraint-error.ll` checking this.
> ```
> // CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>8987   if (OpInfo.ConstraintType == TargetLowering::C_Immediate ||
>8988   OpInfo.ConstraintType == TargetLowering::C_Other) {
>8989 std::vector Ops;
>8990 TLI.LowerAsmOperandForConstraint(InOperandVal, 
> OpInfo.ConstraintCode,
>8991   Ops, DAG);
>8992 if (Ops.empty()) {
>8993   if (OpInfo.ConstraintType == TargetLowering::C_Immediate)
>8994 if (isa(InOperandVal)) {
>8995   emitInlineAsmError(Call, "value out of range for 
> constraint '" +
>8996
> Twine(OpInfo.ConstraintCode) + "'");
>8997   return;
>8998 }
> ```
> 
> 
> For arm/aarch64, if the input is invalid, the function also directly return.
> ```
> // AArch64ISelLowering.cpp
>9583 case 'K':
>9584   if (AArch64_AM::isLogicalImmediate(CVal, 32))
>9585 break;
>9586   return;
>9587 case 'L':
>9588   if (AArch64_AM::isLogicalImmediate(CVal, 64))
>9589 break;
>9590   return;
> ```
> 
> For LoongArch, `TargetLowering::LowerAsmOperandForConstraint` is only called 
> when the constraint is not one of `l/I/K`.
Ah, excellent. And the error is good too, so looks good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134157/new/

https://reviews.llvm.org/D134157

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134157: [Clang][LoongArch] Add inline asm support for constraints f/l/I/K

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:1958
+  // constraints while the official register name is prefixed with a '$'.
+  // So we manually select general purpose registers here.
+  // For now, no need to support ABI names (e.g. `$a0`) as clang will correctly

Can't you just clip the `$`, upper-case and match against some table-gen'd 
names?



Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:2064
+uint64_t CVal = C->getSExtValue();
+if (isInt<16>(CVal))
+  Ops.push_back(

Shouldn't this break if the constant isn't in the right type? What happens if 
it isn't? 

It seems it just doesn't append the operand and return. Wouldn't that just 
break the op's format?

On Arm, the logic is simpler:
 - Iterate through all constraints, validating the input
 - If valie, set the Result and append to the Op at the end
 - Otherwise bail and let `TargetLowering::LowerAsmOperandForConstraint` handle 
it.



Comment at: llvm/test/CodeGen/LoongArch/inline-asm-constraint.ll:5
+; RUN: llc --mtriple=loongarch64 --verify-machineinstrs --no-integrated-as < 
%s \
+; RUN:   | FileCheck --check-prefix=LA64 %s
+

I'm not seeing differences between LA32 and LA64, is splitting the CHECK lines 
really necessary?

On some other tests, too...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134157/new/

https://reviews.llvm.org/D134157

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132285: [Clang][LoongArch] Implement ABI lowering

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

I can't vouch for your ABI correctness, but you seem to have a lot of tests 
covering what you claim to have implemented, so looks good to me.

Thanks for opening an issue for RISC-V.

Clang's TargetInfo.cpp is really large, perhaps we should think about splitting 
it across different targets, but definitely NOT for this review.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132285/new/

https://reviews.llvm.org/D132285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I think we can safely say that we care less about jazelle than we care about 
armv1/2/3, so feel free to ignore that, too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133109/new/

https://reviews.llvm.org/D133109

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.

Agree. Even 10 years ago we made the concerted effort not to care about pre-v4, 
so I'd be a little surprised if people are actually using modern clang to 
target those platforms.

Projects that rely on it can work in the same way as gcc and pick an older 
version of the toolchains.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133109/new/

https://reviews.llvm.org/D133109

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132550: Changes to code ownership in clang and clang-tidy

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.

W00t!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132550/new/

https://reviews.llvm.org/D132550

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130255: [Clang][LoongArch] Add initial LoongArch target and driver support

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

This looks great, thanks!

Exciting that we can finally go all the way from C source to LoongArch binary.

The changes look good to me, other than a few nits. But please wait for a day 
or two for other people to review on their own time.




Comment at: clang/lib/Basic/Targets/LoongArch.h:69
+// TODO: select appropriate ABI.
+ABI = "ilp32d";
+  }

nit: this should use `setABI`. Right now, there's no difference, but once the 
function does more (like setting other ABI flags), you won't need to change it 
here. Same for the 64-bit version.



Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.h:25
+} // end namespace loongarch
+} // namespace tools
+} // end namespace driver

nit: comment mismatch "end"



Comment at: clang/test/Driver/loongarch-abi.c:41
+
+// CHECK-LA32-LP64S: error: unknown target ABI 'lp64s'
+// CHECK-LA32-LP64F: error: unknown target ABI 'lp64f'

please, split between pass and error by adding a new `loongarch-abi-error.c` 
and adding the `error` tests to it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130255/new/

https://reviews.llvm.org/D130255

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126451: [Clang][CSKY] Add support about CSKYABIInfo

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

In D126451#3541590 , @zixuan-wu wrote:

> I don't think it is largely similar to RISCV implementation except for the 
> code structure and test reuse. And the test result is big different.

Sorry, that's what I meant. There are no new things added, except for the RISCV 
specific logic, which makes it very easy to review, so thanks! :D


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126451/new/

https://reviews.llvm.org/D126451

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126451: [Clang][CSKY] Add support about CSKYABIInfo

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

This looks good to me, but wait to make sure others see it, too.

My reasons are: it is largely similar to RISCV implementation, it seems to 
follow what I expected of the ABI (which is similar to other targets) and has a 
large corpus of tests.

I can't comment on the specifics of the ABI implementation (I haven't read the 
ABI document *that* thoroughly), but as David said, issues will be picked up by 
tests until the target reaches maturity.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126451/new/

https://reviews.llvm.org/D126451

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124977: [NFC][Clang] Modify expect of fail test or XFAIL because CSKY align is different

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/test/Sema/builtin-alloca-with-align.c:32
 void test8(void) {
+#if defined(__csky__)
   __builtin_alloca_with_align(sizeof(__INT64_TYPE__), 
__alignof__(__INT64_TYPE__)); // expected-warning {{second argument to 
__builtin_alloca_with_align is supposed to be in bits}}

zixuan-wu wrote:
> rengolin wrote:
> > This test is platform agnostic, perhaps the extra error could be in a new 
> > test, exclusively for csky?
> Then I prefer to split the file and add UNSUPPORTED for CSKY in the other 
> file which only contains test8
But then you wouldn't be testing the extra error you want... hmm.

Maybe it would be fine the way you did it originally.

Would be nice to get a clang person to give their opinion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124977/new/

https://reviews.llvm.org/D124977

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124977: [NFC][Clang] Modify expect of fail test or XFAIL because CSKY align is different

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/test/Sema/builtin-alloca-with-align.c:32
 void test8(void) {
+#if defined(__csky__)
   __builtin_alloca_with_align(sizeof(__INT64_TYPE__), 
__alignof__(__INT64_TYPE__)); // expected-warning {{second argument to 
__builtin_alloca_with_align is supposed to be in bits}}

This test is platform agnostic, perhaps the extra error could be in a new test, 
exclusively for csky?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124977/new/

https://reviews.llvm.org/D124977

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121445: [Clang][CSKY] Add the CSKY target and compiler driver

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

In D121445#3418195 , @zixuan-wu wrote:

> I have met this before because the downloading of patch will ignore empty 
> files. You can have a check that your apply does not contain new empty files 
> in multilib_csky_linux_sdk.

You are correct, it doesn't. :(

> For warnings, those are not introduced by this patch. I create another NFC to 
> remove unused variable and function.

Fair enough.

LGTM, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121445/new/

https://reviews.llvm.org/D121445

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121445: [Clang][CSKY] Add the CSKY target and compiler driver

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I just applied this to a recent HEAD and got a few warnings. Please make sure 
there are no new warnings on changes / new files.

  
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYInstrFormats.td:658:62:
 warning: unused template argument: R_Z_2:pattern
  class R_Z_2 sop, bits<5> pcode, string op, list pattern>
   ^



  In file included from 
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYFrameLowering.cpp:14:
  
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYMachineFunctionInfo.h:21:20:
 warning: private field 'MF' is not used [-Wunused-private-field]
MachineFunction 
 ^



  
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/CSKYInstrInfo.cpp:480:24:
 warning: unused variable 'MRI' [-Wunused-variable]
MachineRegisterInfo  = MBB.getParent()->getRegInfo();
 ^



  
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/Disassembler/CSKYDisassembler.cpp:169:21:
 warning: unused function 'DecodesFPR128RegisterClass' [-Wunused-function]
  static DecodeStatus DecodesFPR128RegisterClass(MCInst , uint64_t RegNo,
  ^
  
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/Disassembler/CSKYDisassembler.cpp:199:21:
 warning: unused function 'DecodeGPRSPRegisterClass' [-Wunused-function]
  static DecodeStatus DecodeGPRSPRegisterClass(MCInst , uint64_t RegNo,
  ^
  
/home/rengolin/devel/llvm-project/llvm/lib/Target/CSKY/Disassembler/CSKYDisassembler.cpp:36:16:
 warning: private field 'inDataRegion' is not used [-Wunused-private-field]
mutable bool inDataRegion = false;
 ^

etc.

There is also a test error:

   TEST 'Clang :: Driver/csky-toolchain.c' FAILED 

  ...
  /home/rengolin/devel/llvm-project/clang/test/Driver/csky-toolchain.c:16:24: 
error: C-CSKY-LINUX-MULTI: expected string not found in input
  // C-CSKY-LINUX-MULTI: 
"{{.*}}/Inputs/multilib_csky_linux_sdk/lib/gcc/csky-linux-gnuabiv2/6.3.0/../../..{{/|}}..{{/|}}csky-linux-gnuabiv2/bin{{/|}}ld"
 ^

I'm guessing this is the path of a local sysroot you have on your machine?

If possible, try to get a new environment (container, VM, alternative machine) 
and make sure a clean build still passes the tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121445/new/

https://reviews.llvm.org/D121445

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121445: [Clang][CSKY] Add the CSKY target and compiler driver

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I'm surprised these tests are passing for you. Perhaps you're not building or 
running them all.

To make sure you're running your tests, you need to build both clang and llvm 
(`-DLLVM_ENABLE_PROJECTS=clang`) and run ninja/make `check-all`.

You can also run `lit` directly on each test, but I can't remember how to do 
that now...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121445/new/

https://reviews.llvm.org/D121445

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121445: [Clang][CSKY] Add the CSKY target and compiler driver

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/test/Driver/csky-arch-error.c:1
+// RUN: %clang -target csky-unknown-elf -march=csky -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=CSKY %s

This will error out and fail the test. You need to add a `not` before `%clang`.

Check tests in `clang/test/Driver/` that has `not %clang` as their RUN lines 
and do the same.

And keep all the `not %clang` tests separate from the `%clang` tests.



Comment at: clang/test/Driver/csky-arch.c:28
+
+// CHECK-NOT: error: invalid arch name '
+// CSKY-TARGET: "-triple" "csky-unknown-unknown-elf"

If the RUN line fails (ex. on invalid arch name), the return value will be 
non-zero and the test will fail, so CHECKing for the absence of errors is no-op.



Comment at: clang/test/Driver/csky-cpus.c:12
+// RUN: %clang -target csky -### -c %s 2>&1 -mcpu=generic1 | FileCheck 
-check-prefix=FAIL-MCPU-NAME %s
+// FAIL-MCPU-NAME: error: the clang compiler does not support '-mcpu=generic1'
+

Same here, separate `not` tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121445/new/

https://reviews.llvm.org/D121445

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121445: [Clang][CSKY] Add the CSKY target and compiler driver

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I don't know enough about your toolchain requirements, but this looks good to 
me.

Please check the clang-format warnings. If you did pass clang-format, perhaps 
you need to upgrade to a newer one?

I won't approve just yet, to let other people review it also.




Comment at: clang/test/Driver/csky-arch.c:26
+
+// RUN: %clang -target csky-unknown-elf -march=csky -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=CSKY %s

I don't think this is doing what you expect it to do.

Depending on the output, you can still match all CHECK lines and not have the 
output you want.

To avoid issues, we usually separate tests that must pass (positive tests) like 
the lines above 24, from tests that must fail (negative tests) like the lines 
below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121445/new/

https://reviews.llvm.org/D121445

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100372: [Clang][ARM] Define __VFP_FP__ macro unconditionally

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.

It's a weird flag, for sure, but if that's the semantics of it, than this 
change LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100372/new/

https://reviews.llvm.org/D100372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88394: [Driver][M68k] (Patch 8/8) Add driver support for M68k

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2089
+  static const char *const M68kTriples[] = {
+  "m68k-linux-gnu", "m68k-unknown-linux-gnu", "m68k-suse-linux"};
+

jrtc27 wrote:
> rengolin wrote:
> > The front-end supports FreeBSD, too.
> Aren't these arrays only used on multiarch systems so irrelevant for BSDs? 
> There aren't FreeBSD triples listed for X86/X86_64 for example.
Makes sense.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88394/new/

https://reviews.llvm.org/D88394

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88394: [Driver][M68k] (Patch 8/8) Add driver support for M68k

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.

LGTM too, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88394/new/

https://reviews.llvm.org/D88394

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88394: [Driver][M68k] (Patch 8/8) Add driver support for M68k

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/include/clang/Driver/Options.td:164
+def m_m68k_Features_Group: OptionGroup<"">,
+ Group, DocName<"M68k">;
 def m_mips_Features_Group : OptionGroup<"">,

craig.topper wrote:
> bruno wrote:
> > Looks like this is missing `clang-format`?
> I don't think clang-format really understands tablegen files.
It does not. Don't clang-format table-gen files, the end result is horrendous. 
:)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88394/new/

https://reviews.llvm.org/D88394

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88393: [cfe][M68k] (Patch 7/8) Basic Clang support

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Thanks for the changes. This looks good to me but I'll let others check again 
and approve.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88393/new/

https://reviews.llvm.org/D88393

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87981: [X86] AMX programming model.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

You should at least get @craig.topper's feedback, given this is a significant 
change in the x86 backend.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87981/new/

https://reviews.llvm.org/D87981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88394: [Driver][M68k] (Patch 8/8) Add driver support for M68k

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/include/clang/Driver/Options.td:3125
+foreach i = {0-4} in
+  def m680#i#0 : Flag<["-"], "m680"#i#"0">, Group;
 

Same question as @RKSimon had below: Shouldn't this cover all models the 
back-end recognises?



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2089
+  static const char *const M68kTriples[] = {
+  "m68k-linux-gnu", "m68k-unknown-linux-gnu", "m68k-suse-linux"};
+

The front-end supports FreeBSD, too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88394/new/

https://reviews.llvm.org/D88394

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88393: [cfe][M68k] (Patch 7/8) Basic Clang support

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/lib/Basic/Targets.cpp:314
+default:
+  return new M68kTargetInfo(Triple, Opts);
+}

No support for bare-metal?



Comment at: clang/lib/Basic/Targets/M68k.cpp:123
+"d0", "d1", "d2", "d3", "d4", "d5", "d6", "d7",
+"a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7"};
+

no "sp", "pc", etc? Or are all of them aliased to those?



Comment at: clang/lib/CodeGen/TargetInfo.cpp:8083
+
+// TODO Does not actually work right now
+void M68kTargetCodeGenInfo::setTargetAttributes(

That's an odd comment... :)

What doesn't work right now? Something or everything?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88393/new/

https://reviews.llvm.org/D88393

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90956: [clang][SVE] Activate macro `__ARM_FEATURE_SVE_VECTOR_OPERATORS`.

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.

Hi @fpetrogalli, the document is so dense that it took me a while to check the 
macros and I was still wrong.

Either I'm losing my skill to read Arm documents or folks are getting worse at 
writing them.

Giving this is a change that only affects SVE targets, and I have no way of 
independently verify codegen, I'm trusting you and the rest of Arm to make sure 
it's the right semantics. :)

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90956/new/

https://reviews.llvm.org/D90956

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90956: [clang][SVE] Additional macros implied by `-msve-vector-bits=`.

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Simple and straightforward, with documentation! :)

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90956/new/

https://reviews.llvm.org/D90956

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83088: Introduce CfgTraits abstraction

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I have read all of the comments in this review and have looked at all the other 
pending reviews because of this and I still see strong disagreement on the 
design and implementation.

Unfortunately, I can't contribute with the technical discussion because there's 
a lot more context and content here than I can absorb in the time I have 
available, but overall I think David's critics are very pertinent. They don't 
mean the code is wrong or bad, just that they are important questions that need 
answers. Some of the questions were answered, others weren't. This patch should 
not have been committed before those things were sorted out, as is clearly 
stated in the (existing) review policy 
(https://llvm.org/docs/CodeReview.html#acknowledge-all-reviewer-feedback).

I do appreciate that the other patches are "waiting" for this one and that it 
has been months, but this patch fundamentally changes the algorithm with a 
motivation that still isn't clear for two reasons:

1. It was initially stated that the motivation is to reduce the number of 
templates because the author doesn't like them, which is not a good reason.
2. Despite recurrent requests to show concrete code examples comparing current 
and new design, showing why it would be "easier to use", none have materialised 
(other than David's own attempts).

All of the other patches were approved by the same set of people. This is 
definitely not uncommon, but is highly susceptible to unconscious bias. Once 
David questioned the approach with concrete questions, concrete answers should 
address all of the points.

This patch should have never been committed in the first place, even with one 
approval.

In D83088#2342760 , @dblaikie wrote:

> Sorry about the delays in review - but please revert this patch until we can 
> hash out a few more details. I really don't think this is the best direction 
> forward for a core abstraction & I'll do my best to explain why & try to 
> understand where you're coming from.

The (current) review policy 
(https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed)
 is already clear enough:

"There is a strong expectation that authors respond promptly to post-commit 
feedback and address it. Failure to do so is cause for the patch to be 
reverted."

It's pretty clear that the paragraph applies to David's post-commit review.

> I'd really like to see examples like this ^ using the different abstractions 
> under consideration here (classic GraphTraits, CfgTraits dynamic and static 
> typed, perhaps what a static API would look like if it wasn't trying to be 
> dynamic API compatible).

This is the main request that was left unaddressed and the one that has the 
highest impact on the design of the API as well as all the following patches. 
The fact that they have all been approved doesn't mean this one must, too.

Their own approval just means those changes look good with the current 
interpretation, not that they must land. It's entirely possible that they 
continue to be good after the API has changed (if it has), in which case the 
approvals can just be restated. But they may well disappear or change 
completely due to the change in API, and will need new reviews to avoid having 
an already approved review change substantially in content.

> Indeed template code can get a bit weird - but I'm not sure it's quite enough 
> to justify the change in/complications of mulitple (static and dynamic) 
> abstractions here just yet. It might be that taking a more structured C++ 
> idiomatic approach to template design (like C++ standard container concept 
> abstractions) might lead to more usable code without /some/ complexities 
> (though may trade others).

And this is the main critic to the overall design choice, which I also agree 
completely. I'm not a big fan of complex template code myself, but the idioms 
are well known and they do simplify usage in many cases.

LLVM has had its fair share of discussions on the topics and we have developed 
multiple APIs and containers tho overcome deficiencies in the standard library, 
some of those concepts made into the standard. Some of those I have learned to 
like when I tried to implement otherwise and failed.

Only with concrete comparison of usage and patterns that we can make an 
informed decision and this is required to change a core concept of the compiler 
more than any other place. A single example where your pattern fits isn't 
enough to demonstrate that it will be generic and usable for all the other 
patterns that it may be used.

I'm sorry this delays more your work, but this is what working on an open 
source very large project entails. In comparison, LLVM is very fast compared to 
other OSS projects out there.

Also, echoing other people in this thread, this is more a case for an RFC 
thread on the list than code review. If the original thread didn't catch the 
attention of a public 

[PATCH] D88154: Initial support for vectorization using Libmvec (GLIBC vector math library).

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1564
 def fveclib : Joined<["-"], "fveclib=">, Group, Flags<[CC1Option]>,
-HelpText<"Use the given vector functions library">, 
Values<"Accelerate,MASSV,SVML,none">;
+HelpText<"Use the given vector functions library">, 
Values<"Accelerate,LIBMVEC,MASSV,SVML,none">;
 def fno_lax_vector_conversions : Flag<["-"], "fno-lax-vector-conversions">, 
Group,

abique wrote:
> I think glibc always refer to the library as "libmvec" in lower case,  should 
> we do so here?
Yes, so clang can be a drop in replacement. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88154/new/

https://reviews.llvm.org/D88154

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87338: [Driver][PGO] Driver support for de-Optimizing cold functions (PATCH 2/2)

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2021
+
+Adds ``optnone`` attributes to functions whose instrumentation-based PGO 
profiling counts are equal to zero (i.e. "cold").
+

Following discussions in the mailing list, I'm wondering if we want to join the 
`optsize` and `optnone` implementations into one flag.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87338/new/

https://reviews.llvm.org/D87338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

+1 to Chris, Mehdi and John comments. I think I get the idea, but the text is 
certainly not conveying that, for me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77683/new/

https://reviews.llvm.org/D77683



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65572: Fix static linking failure with --unwindlib=libunwind

[Renato Golin via Phabricator via cfe-commits]

rengolin added reviewers: compnerd, mstorsjo, jroelofs, theraven.
rengolin added a comment.

This is a tricky one which may vary depending on the libraries available on 
different systems. Which toolchain is this? Can you add more context?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65572/new/

https://reviews.llvm.org/D65572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65404: [AArch64] Disable __ARM_FEATURE_SVE without ACLE.

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

I see, makes sense. LGTM, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65404/new/

https://reviews.llvm.org/D65404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65404: [AArch64] Disable __ARM_FEATURE_SVE without ACLE.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I understand (and agree with) the reasoning of this patch, but wouldn't this 
also make it harder to test the current behaviour?

I mean, LLVM doesn't support officially SVE entirely, so there's no point in 
expecting anything to work for now. What is the coverage of the current ACLE 
implementation in Clang/LLVM?

If we already have the most used ones, how about adding a warning that SVE 
support is not complete, YMMV, etc. instead?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65404/new/

https://reviews.llvm.org/D65404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23610: [ARM] Add pre-defined macros for ROPI and RWPI

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Sorry for the delay, this fell out of my radar and just saw the ping now.

Given it's in ACLE and there are only mechanical (obvious) changes, LGTM.

I'm assuming those two parameters are already accepted in the LLVM back-end, 
right?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D23610/new/

https://reviews.llvm.org/D23610



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53928/new/

https://reviews.llvm.org/D53928



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.
Herald added a subscriber: jdoerfert.

Funny thing is, SVML is also only supported, AFAIK, for Intel. I agree that we 
should emit errors, but we should also emit a similar error on SVML.

I know it's not entirely relevant to this patch, but we should keep the 
behaviour consistent on all veclibs.

Also, can you add a test for the new error messages, please?

The new changes look good to me and don't substantially deviate from the 
previous, approved, patch, so LGTM after addressing the comments.

Thanks!




Comment at: lib/Frontend/CompilerInvocation.cpp:678
+  else
+Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args)
+  << Name;

This is not really invalid value for the flag, it's invalid architecture for 
the value.

I think there's already a string for that somewhere in Clang.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53928/new/

https://reviews.llvm.org/D53928



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

@hfinkel There are changes in this patch, as well as its LLVM counterpart 
D53927 . Please, re-review.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53928/new/

https://reviews.llvm.org/D53928



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions

[Renato Golin via Phabricator via cfe-commits]

rengolin requested changes to this revision.
rengolin added a comment.
This revision now requires changes to proceed.

Since the previous version was approved already, I'm "requesting changes" so 
that we can look at it again, together with D53927 
 to make sure it's still the right way to go.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53928/new/

https://reviews.llvm.org/D53928



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53928: Enable builtins necessary for SLEEF [AArch64] vectorized trigonometry libm functions

[Renato Golin via Phabricator via cfe-commits]

rengolin added reviewers: rsmith, chandlerc, rnk, ABataev.
rengolin added a comment.

Adding clang/omp developers for proper review. Please feel free to add more.

FYI, there are four main ongoing discussions on the LLVM thread (D53927 
):

1. There is collusion between clang pragmas (`clang vectorise`), OpenMP pragmas 
(`omp simd`) and newly proposed `veclib` pragmas, which IMHO is too much. We 
need more OpenMP people looking at them. My proposal is to not add new ones and 
use OpenMP SIMD without creating library dependencies with OMP, but the 
interpretation of other pragmas in the same source needs sorting.

2. How we're going to handle the naming scheme, currently proposed to have a 
local `math.h` and mask the SLEEF includes with macros. My proposal is to have 
a special header that the compiler only includes upon `-mveclibabi` instead. 
Library includes end up as `-lsleef` in the linker command line.

3. Who controls the header file. The proposal is Clang control the headers for 
all vector libraries (all in `math.h` with guards), which means the evolution 
of Clang and each library is now tied. My proposal is for the library to have 
their own headers in a standard place (`/usr/include/arch/something') and get 
Clang to agree on the right place, so that it can include the header from 
`$default_path/$library_name.h`.

4. The option was called `fveclib` but GCC calls it `mveclibabi`. There doesn't 
seem to be any behavioural differences, so I think we should follow the same 
name. We may be forced to create an alias, because this is not new in LLVM 
(already existed for SVML). This can go on a separate patch. But it's good to 
be aware of that. Making it an alias, we are fixing the implementation to be at 
least compatible with GCC, which is a good thing.

cheers,
--renato




Comment at: lib/Index/CommentToXML.cpp:230
   // Inline content.
-  void visitTextComment(const TextComment *C);
-  void visitInlineCommandComment(const InlineCommandComment *C);
-  void visitHTMLStartTagComment(const HTMLStartTagComment *C);
-  void visitHTMLEndTagComment(const HTMLEndTagComment *C);
+  void VisitTextComment(const TextComment *C);
+  void VisitInlineCommandComment(const InlineCommandComment *C);

Spurious change and unrelated to the patch. Please revert this part.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53928/new/

https://reviews.llvm.org/D53928



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Hi Peter,

Looks ok. Why did you need to change all cmdlines to have -EL? I imagine you 
just need one for each case, everything else remains the default (which should 
still work).

Also, it would be interesting to know what happens on cases like "aarch64_be 
-EL" et al, and have negative tests for them.

cheers,
--renato


https://reviews.llvm.org/D52784



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52595: [ARM] Alter test to account for change to armv6k default CPU

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Thanks Peter. LGTM!


https://reviews.llvm.org/D52595



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a subscriber: joerg.
rengolin added a comment.

LTO is something I never considered before in the context of the target parser, 
but I understand the issues are similar to what the build attributes were 
trying to solve, so adding more info shouldn't make it worse.

However, this will probably break obscure cases that normally only @compnerd 
and @joerg pick up. I, personally, don't have any opinion on how much this 
makes sense or not. :/

I'm with @fhahn that we should make ARM and AArch64 as common as possible, not 
only in code for the target parser but also in behaviour for the front-end, 
mostly because ARMv8 spans both and we want to avoid surprises.


Repository:
  rC Clang

https://reviews.llvm.org/D45240



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40256: [ARM] disable FPU features when using soft floating point.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Wasn't that the mess about -mfpu=softfp?


https://reviews.llvm.org/D40256



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

In https://reviews.llvm.org/D38479#886587, @efriedma wrote:

> 1. We don't correctly ignore inline asm clobbers for registers which aren't 
> allocatable (https://bugs.llvm.org/show_bug.cgi?id=30792)


This looks like a different (but related) issue. That should be fixed in the 
back-end, regardless of the new error messages.

> 2. We don't diagnose calls which need vector registers according to the C 
> calling convention.

The function that checks for it returns true for vectors 
(`lib/Sema/SemaExprCXX.cpp:7487`). However, the tests cover floating point, but 
they don't cover vector calls (arm_neon.h).

> 3. We don't diagnose operations which have to be lowered to libcalls which 
> need vector registers according to the C calling convention (fptosi, 
> @llvm.sin.*, etc.).

Yup. That's bad.

> All three of these could be addressesed in the AArch64 backend in a 
> straightforward manner.

I worry about declarations and front-end optimisations, which may move the line 
info to a completely different place (where it's used, for example). But I have 
no basis for that worry. :)

> Diagnosing floating-point operations in Sema in addition to whatever backend 
> fixes we might want is fine, I guess, but I don't really like making 
> "-mgeneral-regs-only" into "-mno-implicit-float" plus some diagnostics; other 
> frontends don't benefit from this checking, and using no-implicit-float is 
> asking for an obscure miscompile if IR generation or an optimization 
> accidentally produces a floating-point value.

I agree with both statements. But it would be good to have the errors in Sema 
in addition to the back-end fixes, if there are cases where Clang's diagnostics 
is more precise on line info and carat position.




Comment at: lib/Sema/SemaExprCXX.cpp:7477
+static bool typeHasFloatingOrVectorComponent(
+QualType Ty, llvm::SmallDenseMap ) {
+  if (Ty.isNull())

The `TypeCache` object seems local.

It doesn't look like it needs to survive outside of this function, as per its 
usage and the comment:

// We may see recursive types in broken code.

and it just adds another argument passing.


https://reviews.llvm.org/D38479



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36731: [ARM][AArch64] Cortex-A75 and Cortex-A55 tests

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I'm happy with the patch, but I'll let @SjoerdMeijer approve.


https://reviews.llvm.org/D36731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36731: [ARM][AArch64] Cortex-A75 and Cortex-A55 support

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

This change seems to have nothing to do with adding core support but exposing 
features from CPU names.

If this was required to "support" the new cores in Clang, why wasn't it needed 
before?

If this is a new, more generic way, of getting support, shouldn't you remove 
the specialist code that already does that for all other cores?

cheers,
--renato




Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:90
 
+static bool DecodeARMFeaturesFromCPU(const Driver , StringRef CPU,
+ std::vector ) {

Why are you returning bool if you're not using the result?



Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:348
 Args.MakeArgString((F.second ? "+" : "-") + F.first()));
+  } else if (!CPUName.empty() && CPUName != "generic") {
+DecodeARMFeaturesFromCPU(D, CPUName, Features);

Isn't this conditional redundant with what the function does?


https://reviews.llvm.org/D36731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35826: [Driver] Error if ARM mode was selected explicitly for M-profile CPUs.

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

This makes sense to me. LGTM. Thanks!


https://reviews.llvm.org/D35826



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35118: [AArch64] Add support for handling the +sve target feature

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Thanks. LGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D35118



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35118: [AArch64] Add support for handling the +sve target feature

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

In https://reviews.llvm.org/D35118#807712, @aemerson wrote:

> In https://reviews.llvm.org/D35118#806730, @rengolin wrote:
>
> > @jmolloy Can you check this change, please?
>
>
> I'm not really removing FPUMode, I'm just converting an enum to a bit field. 
> FPUMode, i.e. no NEON, after this change is now represented by simply having 
> all bits be 0. See the equivalent implementation for ARM which does the same 
> thing.


That's not really a bit field, but the point is that you're removing the 
explicit categorisation, which helps people understand what it all means and 
why it was there in the first place.

There is no reason to remove FPUMode. You can just add "SveMode" to that list 
and make it (1 << 1) to make it explicit that this is bit pattern enum.


Repository:
  rL LLVM

https://reviews.llvm.org/D35118



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35118: [AArch64] Add support for handling the +sve target feature

[Renato Golin via Phabricator via cfe-commits]

rengolin added a reviewer: jmolloy.
rengolin added a subscriber: jmolloy.
rengolin added a comment.

@jmolloy Can you check this change, please?




Comment at: lib/Basic/Targets.cpp:6245
   enum FPUModeEnum {
-FPUMode,
-NeonMode
+NeonMode = (1 << 0),
+SveMode = (1 << 1)

aemerson wrote:
> rengolin wrote:
> > Is there any AArch64 arch without SIMD?
> > 
> > Anyway, that seems deliberate, @t.p.northover any idea?
> SIMD support can be disabled with +nosimd. This change doesn't affect how 
> that works.
The commit that introduced this (by James) states: "Allow the disabling of NEON 
and crypto instructions."

I wouldn't remove the FPUMode as is.

It's also surprising that this passes the test that James updated on the same 
commit. Maybe the tests weren't good enough?


Repository:
  rL LLVM

https://reviews.llvm.org/D35118



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35118: [AArch64] Add support for handling the +sve target feature

[Renato Golin via Phabricator via cfe-commits]

rengolin added a reviewer: t.p.northover.
rengolin added a subscriber: t.p.northover.
rengolin added inline comments.



Comment at: lib/Basic/Targets.cpp:6245
   enum FPUModeEnum {
-FPUMode,
-NeonMode
+NeonMode = (1 << 0),
+SveMode = (1 << 1)

Is there any AArch64 arch without SIMD?

Anyway, that seems deliberate, @t.p.northover any idea?


Repository:
  rL LLVM

https://reviews.llvm.org/D35118



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32427: Fix float abi for SUSE ARM triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added reviewers: compnerd, rovka, joerg.
rengolin added a comment.

I'm not sure this will work at all. Not because it doesn't make sense (it 
does), but because of several bugs in the soft vs. hard float implementation in 
the Triple related classes all the way to the back-end.

I'm adding other folks to discuss.


https://reviews.llvm.org/D32427



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32347: Add support for openSUSE ARM Triples

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Ok, I see no harm in letting you work upstream, since no one should be relying 
on the correct behaviour on opensuse for now. LGTM. Thanks!


https://reviews.llvm.org/D32347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32347: Add support for openSUSE ARM Triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added reviewers: rovka, compnerd.
rengolin added a comment.

In https://reviews.llvm.org/D32347#733475, @ismail wrote:

> Yes openSUSE prefers the "hl" prefix to mean HardFloat.


I'm not sure that'll do what you expect it, though. Having the "hf" at the end 
of the triple has some magical effect on the Triple class.

But at the very least, you should make sure that `float-abi` is indeed `hard`.

I'm adding Diana and Saleem, who have had more exposure to float abi 
shenanigans than I have, recently.


https://reviews.llvm.org/D32347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32347: Add support for openSUSE ARM Triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Nothing new here, pretty much standard. No "gnueabihf"?


https://reviews.llvm.org/D32347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32132: [AArch64][clang] Pass cpu/arch information to assembler for AArch64.

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Silly omission. LGTM. Thanks!


https://reviews.llvm.org/D32132



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31417: [OpenMP] Add support for omp simd pragmas without runtime

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Hi Graham,

It looks much simpler now, thanks!

I'm ok with the change, but I'd like @ABataev to confirm that the semantics is 
the expected one for all cases and approve.

cheers,
--renato




Comment at: lib/Parse/ParseOpenMP.cpp:174
+  case OMPD_target_teams_distribute_simd:
+DKind = OMPD_simd;
+break;

I'd like @ABataev to confirm this is the right semantics.



Comment at: lib/Parse/ParseOpenMP.cpp:1047
+// as the filter function will have switched the kind.
+if (!getLangOpts().OpenMPSimd)
+  Diag(Tok, diag::err_omp_unknown_directive);

What if it's really unknown, even to `-fopenmp-simd`?


https://reviews.llvm.org/D31417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31417: [OpenMP] Add support for omp simd pragmas without runtime

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

In https://reviews.llvm.org/D31417#713171, @huntergr wrote:

> The other alternative I thought of was to perform the filtering in 
> ParseOpenMP.cpp instead, but I need to figure out how to delete or skip 
> tokens there without cluttering up the rest of the OpenMP parsing.


That was my first thought, yes, but I'm not sure how invasive this could get.

> This feature comes from user requests, and basically matches the 
> functionality of other compilers (e.g. gcc).

Right, in this case, I think we have a stronger motivation to make that an 
integral part of the existing OpenMP parser.

cheers,
--renato


https://reviews.llvm.org/D31417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31417: [OpenMP] Add support for omp simd pragmas without runtime

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Hi Graham,

I don't know much about Clang's machinery, but would it be possible to have 
`-fopenmp-simd` generate the same handler, but with restrictions? I fear this 
slight duplication could get considerably worse as we support more and more 
"non-RT" OMP pragmas.

Alternatively, if this is for testing purposes, we have another pragma which 
does exactly the same thing as `omp simd`, which are the Clang vectorizer 
pragmas (http://llvm.org/docs/Vectorizers.html#pragma-loop-hint-directives).

cheers,
--renato


https://reviews.llvm.org/D31417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31178: [libcxxabi] Fix exception address alignment test for EHABI

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

This looks like a simple oversight from my perspective, so looks good.

But I'll let the others have a look at it, as I'm not overly familiar with 
libcxxabi.

cheers,
--renato


https://reviews.llvm.org/D31178



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30739: [OpenMP] "declare simd" for AArch64 Advanced SIMD.

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:6821
+  ISADataTy ISAData[] = {
+  {'n', 64},  // double-word Advanced SIMD
+  {'n', 128}, // quad-word Advanced SIMD

fpetrogalli wrote:
> rengolin wrote:
> > No f32?
> Sorry, I am not sure to understand what you mean here by f32.
> 
> I am considering the double-word registers (64 bits wide) and quad-word 
> registers (128 bits wide) that are used in AdvSIMD (NEON) for AArch64 [1].
> 
> [1] 
> https://developer.arm.com/docs/dui0473/latest/neon-programming/neon-views-of-the-extension-register-bank
Sorry, I meant S0~Sn, but that's silly, as it's VFP. Ignore me.


https://reviews.llvm.org/D30739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30739: [OpenMP] "declare simd" for AArch64 Advanced SIMD.

[Renato Golin via Phabricator via cfe-commits]

rengolin added reviewers: Hahnfeld, carlo.bertolli, arpith-jacob.
rengolin added a comment.

Looks ok to me, but I'm not very knowledgeable in that area. Hopefully some 
OpenMP Clang developers I added could give you a more concrete approval. :)




Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:6821
+  ISADataTy ISAData[] = {
+  {'n', 64},  // double-word Advanced SIMD
+  {'n', 128}, // quad-word Advanced SIMD

No f32?


https://reviews.llvm.org/D30739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30582: [Driver] Restructure handling of -ffast-math and similar options

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Ok, with the break fix, this looks goof to me. Thanks!




Comment at: lib/Driver/ToolChains/Clang.cpp:2452
+  if (!HonorInfs && !HonorNans && !MathErrno && AssociativeMath &&
+  ReciprocalMath && !SignedZeros && !TrappingMath && FpContract == "fast")
+CmdArgs.push_back("-ffast-math");

john.brawn wrote:
> rengolin wrote:
> > rengolin wrote:
> > > This is technically correct, but users will be confused if they choose 
> > > `-ffast-math -ffp-contract=on` and not see `-ffast-math` coming out on 
> > > the other side.
> > > 
> > > Also, `fp-contract=on` doesn't preclude `-ffast-math` for the languages 
> > > that support it, so I wouldn't add `FpContract` to this list at all.
> > I've been thinking a bit more about this, and I started wondering, why do 
> > we even need to pass `-ffast-math` down?
> > 
> > If all the others are already being passed, shouldn't this flag be 
> > redundant?
> > 
> > Finally, we could possibly add instead `&& FpContract != "off"`. Would that 
> > be better?
> As the comment above says, -ffast-math enables the __FAST_MATH__ macro.
> 
> As to FpContract, going back and checking gcc setting -ffp-contract has no 
> effect on whether __FAST_MATH__ is defined so I think just not checking 
> FpContract here is correct.
Right, makes sense.



Comment at: lib/Driver/ToolChains/Clang.cpp:2347
+// Validate and pass through -fp-contract option.
+case options::OPT_ffp_contract: {
+  StringRef Val = A->getValue();

john.brawn wrote:
> rengolin wrote:
> > Also, when `-ffast-math` is selected, and `-ffp-contract=on`, we should 
> > actually change it to `fast`, no?
> Do you mean "clang -ffp-contract=on -ffast-math should set fp-contract to 
> fast" or "clang -ffast-math -ffp-contract=on should set fp-contract to fast"? 
> The first is already done by the -ffast-math handling below, and I think the 
> second is a bad idea because it violates the principle that later 
> command-line options have priority over earlier ones.
The former is done by overall design, the latter is different to most other 
options because of what "on" means and how it's *not* implemented, meaning 
it'll be effectively "off". Otherwise, I'd completely agree with you.

But that's not a strong point, for me. I'm happy to not have that now and 
involve this into a more general "what does on mean" later on.


Repository:
  rL LLVM

https://reviews.llvm.org/D30582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30582: [Driver] Restructure handling of -ffast-math and similar options

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:2452
+  if (!HonorInfs && !HonorNans && !MathErrno && AssociativeMath &&
+  ReciprocalMath && !SignedZeros && !TrappingMath && FpContract == "fast")
+CmdArgs.push_back("-ffast-math");

rengolin wrote:
> This is technically correct, but users will be confused if they choose 
> `-ffast-math -ffp-contract=on` and not see `-ffast-math` coming out on the 
> other side.
> 
> Also, `fp-contract=on` doesn't preclude `-ffast-math` for the languages that 
> support it, so I wouldn't add `FpContract` to this list at all.
I've been thinking a bit more about this, and I started wondering, why do we 
even need to pass `-ffast-math` down?

If all the others are already being passed, shouldn't this flag be redundant?

Finally, we could possibly add instead `&& FpContract != "off"`. Would that be 
better?



Comment at: lib/Driver/ToolChains/Clang.cpp:2347
+// Validate and pass through -fp-contract option.
+case options::OPT_ffp_contract: {
+  StringRef Val = A->getValue();

Also, when `-ffast-math` is selected, and `-ffp-contract=on`, we should 
actually change it to `fast`, no?



Comment at: lib/Driver/ToolChains/Clang.cpp:2356
+}
+
+case options::OPT_ffinite_math_only:

Missing break?


Repository:
  rL LLVM

https://reviews.llvm.org/D30582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30582: [Driver] Restructure handling of -ffast-math and similar options

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:2320
+
+  for (Arg *A : Args)
+  {

format



Comment at: lib/Driver/ToolChains/Clang.cpp:2324
+{
+// Options controlling individual features
+case options::OPT_fhonor_infinities:HonorInfs = true;break;

default should be the first item, for readability.



Comment at: lib/Driver/ToolChains/Clang.cpp:2382
+  if (!OFastEnabled)
+continue;
+case options::OPT_ffast_math:

Use `LLVM_FALLTHROUGH`



Comment at: lib/Driver/ToolChains/Clang.cpp:2452
+  if (!HonorInfs && !HonorNans && !MathErrno && AssociativeMath &&
+  ReciprocalMath && !SignedZeros && !TrappingMath && FpContract == "fast")
+CmdArgs.push_back("-ffast-math");

This is technically correct, but users will be confused if they choose 
`-ffast-math -ffp-contract=on` and not see `-ffast-math` coming out on the 
other side.

Also, `fp-contract=on` doesn't preclude `-ffast-math` for the languages that 
support it, so I wouldn't add `FpContract` to this list at all.


Repository:
  rL LLVM

https://reviews.llvm.org/D30582



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30290: [libcxx][zorg] Fix no-exceptions builder configurations

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!

I'll add a local task to look into that, but with Connect coming, I'm not sure 
how long that will take. :)


https://reviews.llvm.org/D30290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30290: [libcxx][zorg] Fix no-exceptions builder configurations

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: buildbot/osuosl/master/config/builders.py:1196
+lit_extra_opts={'link_flags': '"-lc++abi -lc -lm -lpthread -ldl 
-L/opt/llvm/lib/clang/3.9.0/lib/linux -lclang_rt.builtins-armhf"'},
+cmake_extra_opts={'LIBCXX_ENABLE_EXCEPTIONS': 'OFF',
+  'LIBCXXABI_ENABLE_EXCEPTIONS': 'OFF',

rmaprath wrote:
> rengolin wrote:
> > Why remove the unwinder?
> The unwinder should not be required for the no-exceptions library testing, as 
> these libraries do not throw/catch any exceptions.
> 
> / Asiri
But the requirement is not only to test it, but to make them work. We had 
trouble making the bot pass without the unwinder, due to dependencies.

Regardless, this is a *different* change and should be made later, with proper 
research. For now, just add the new flag, please.


https://reviews.llvm.org/D30290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30290: [libcxx][zorg] Fix no-exceptions builder configurations

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: buildbot/osuosl/master/config/builders.py:1196
+lit_extra_opts={'link_flags': '"-lc++abi -lc -lm -lpthread -ldl 
-L/opt/llvm/lib/clang/3.9.0/lib/linux -lclang_rt.builtins-armhf"'},
+cmake_extra_opts={'LIBCXX_ENABLE_EXCEPTIONS': 'OFF',
+  'LIBCXXABI_ENABLE_EXCEPTIONS': 'OFF',

Why remove the unwinder?


https://reviews.llvm.org/D30290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28820: Warn when calling a non interrupt function from an interrupt on ARM

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Seems like a very specific corner case on ARM, but is that attribute guaranteed 
to be ARM-only?

If so, LGTM as is. If not, avoid mentioning "VFP" on the error message.


https://reviews.llvm.org/D28820



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28238: [Driver] Add openSuse AArch64 Triple

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a reviewer: rengolin.
rengolin added a comment.
This revision is now accepted and ready to land.

Sounds good. Thanks!


https://reviews.llvm.org/D28238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28238: [Driver] Add openSuse AArch64 Triple

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: test/Driver/linux-ld.c:623
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=arm64-unknown-linux-gnu \
+// RUN: --gcc-toolchain="" \

cryptoad wrote:
> rengolin wrote:
> > shouldn't you have used your triple?
> The similar Fedora and Ubuntu tests appear to be using an unknown target as 
> well, I based myself on that.
This is odd... Just be sure the test doesn't pass without your patch.


https://reviews.llvm.org/D28238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28238: [Driver] Add openSuse AArch64 Triple

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: test/Driver/linux-ld.c:623
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=arm64-unknown-linux-gnu \
+// RUN: --gcc-toolchain="" \

shouldn't you have used your triple?


https://reviews.llvm.org/D28238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28238: [Driver] Add openSuse AArch64 Triple

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Please, add tests to test/Driver.


https://reviews.llvm.org/D28238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28078: [compiler-rt] [tests] Add missing "int_lib.h" includes and extend guards

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


https://reviews.llvm.org/D28078



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: lib/builtins/floatuntitf.c:73
+long_double_bits fb;
+fb.u.high.all = (du_int)(e + 16383) << 48/* exponent */
+  | ((a >> 64) & 0xLL);  /* mantissa */

rengolin wrote:
> mgorny wrote:
> > rengolin wrote:
> > > nit: an hexadecimal pattern here would be clearer. same above.
> > Do you mean something like: `(foo << 48) & 0x`?
> No, just the `16383` to `0x3FFF`
Though, now I think your proposal may be better. :)

Whatever works, I'm not too concerned about that.


https://reviews.llvm.org/D27898



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: lib/builtins/floatuntitf.c:73
+long_double_bits fb;
+fb.u.high.all = (du_int)(e + 16383) << 48/* exponent */
+  | ((a >> 64) & 0xLL);  /* mantissa */

mgorny wrote:
> rengolin wrote:
> > nit: an hexadecimal pattern here would be clearer. same above.
> Do you mean something like: `(foo << 48) & 0x`?
No, just the `16383` to `0x3FFF`


https://reviews.llvm.org/D27898



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

My floating-point-fu is lacking, so I'll trust you guys that this makes sense. 
:)

I didn't see anything egregious, so LGTM with the nit inline.

cheers,
--renato




Comment at: lib/builtins/floatuntitf.c:73
+long_double_bits fb;
+fb.u.high.all = (du_int)(e + 16383) << 48/* exponent */
+  | ((a >> 64) & 0xLL);  /* mantissa */

nit: an hexadecimal pattern here would be clearer. same above.


https://reviews.llvm.org/D27898



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27123: Add AVR target and toolchain to Clang

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

In https://reviews.llvm.org/D27123#617118, @jroelofs wrote:

> In https://reviews.llvm.org/D27123#616887, @saaadhu wrote:
>
> > Make defines for CHAR16_TYPE, {U,}INT_{LEAST,FAST}16_TYPE use int instead 
> > of short.
> >
> > {U,}INT16_TYPE still gets defined as short though - 
> > lib/Frontend/InitPreprocessor.cpp::DefineExactWidthIntType does not use 
> > TargetInfo::getIntTypeByWidth. Instead, InitializePredefinedMacros calls 
> > the function with the specific type (SignedShort/UnsignedShort), as 
> > getShortWidth() > getCharWidth(), but getIntWidth() == getShortWidth(). Not 
> > sure what the best way to fix that is - should I make DefineExactWidthType 
> > use TargetInfo::getIntTypeByWidth?
>
>
> I'm not sure either. I think it's a good question for @rengolin.


Hum, sorry, can't answer that one. :)

I find it confusing that you define `CHAR16` as `int` and `INT16` as `short`, 
with whatever `INT_FAST16` is as `int`.

I don't know much about AVR, but the C standard predicts that `short` and `int` 
can have the same size, so Clang should be able to cope with it.

cheers,
--renato




Comment at: lib/Basic/Targets.cpp:8543
+  case llvm::Triple::avr:
+return new AVRTargetInfo(Triple, Opts);
   case llvm::Triple::bpfeb:

dylanmckay wrote:
> If we build clang without `LLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR`, how will 
> this work?
> 
> Will it fail compilation? Will `clang` report that AVR is supported? Will it 
> crash if you try and run `clang`?
AFAIK, Clang support for targets is independent of LLVM. A standard Clang build 
supports all targets up to LLVM IR (so all ABIs, PCSs, type descriptions, 
etc.), but it will fail if the back-end is not available and Clang is invoked 
beyond IR.

This is why we can only have IR tests in Clang. As long as all your tests stop 
at IR, this should be fine.


https://reviews.llvm.org/D27123



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26796: [Driver] Use arch type to find compiler-rt libraries (on Linux)

[Renato Golin via Phabricator via cfe-commits]

rengolin resigned from this revision.
rengolin removed a reviewer: rengolin.
rengolin added a comment.

I don't know enough about the x86 environment to be able to review this patch. 
Sorry.


https://reviews.llvm.org/D26796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27473: Bring back note about not supporting global register variables

[Renato Golin via Phabricator via cfe-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


https://reviews.llvm.org/D27473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24084: [CMake] Cleanup libunwind lookup code.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

So, we found this issue with the different unwinds between Clang and libunwind:

https://llvm.org/bugs/show_bug.cgi?id=31035

Is this related?


https://reviews.llvm.org/D24084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24083: [CMake] Fix libc++abi __aeabi_idiv() link error.

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Was this abandoned? Merged?


https://reviews.llvm.org/D24083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16901: [Clang driver, ARM] Do not add +long-calls in PIC mode

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Was this abandoned?


Repository:
  rL LLVM

https://reviews.llvm.org/D16901



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23610: [ARM] Add pre-defined macros for ROPI, RWPI and FPIC

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

Was this abandoned?


Repository:
  rL LLVM

https://reviews.llvm.org/D23610



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26887: [Driver] Fix finding multilib gcc install on Gentoo (with gcc-config)

[Renato Golin via Phabricator via cfe-commits]

rengolin resigned from this revision.
rengolin removed a reviewer: rengolin.
rengolin added a comment.

I don't know enough about x86_64's ABI to have a solid opinion.


https://reviews.llvm.org/D26887



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25686: [Driver] Improve support for Gentoo arm*-hardfloat-*-*eabi triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

We already massage the triple in many cases, and what goes into IR is not 
always the same as what is used in Clang.

Examples are x86_64's "x32" extension to the ABI, ARM -> Thumb triples, adding 
compulsory dashes and unknowns (ex. x86_64--linux-gnu), etc.

You just have to find the right places.


https://reviews.llvm.org/D25686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25686: [Driver] Improve support for Gentoo arm*-hardfloat-*-*eabi triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

I think this patch should move to canonicalise the triple as per all the 
others, in the right place (vendor/environment).

If you find `hardfloat` or on where the vendor should be, and there are no 
other possible vendors, just append `hf` to the environment and mark the vendor 
as `unknown`. This works already with `gnueabihf`, `eabihf` and `muslabihf` 
(doubts about this last one).

What the clang driver accepts is different than what the rest of the compiler 
should use.


https://reviews.llvm.org/D25686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25686: [Driver] Improve support for Gentoo arm*-hardfloat-*-*eabi triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: test/Driver/linux-ld.c:1016
+// CHECK-LD-GENTOO-ARMHF: "-dynamic-linker" "/lib/ld-linux-armhf.so.3"
+// CHECK-LD-GENTOO-ARMHF: 
"{{.*}}/usr/lib/gcc/armv7a-hardfloat-linux-gnueabi/4.9.3/crtbegin.o"
+// CHECK-LD-GENTOO-ARMHF: 
"-L[[SYSROOT]]/usr/lib/gcc/armv7a-hardfloat-linux-gnueabi/4.9.3"

mgorny wrote:
> rengolin wrote:
> > mgorny wrote:
> > > rengolin wrote:
> > > > where is 4.9.3 coming from?
> > > It's the version used in the input tree, i.e. the two added files above.
> > Right, I mean is this "4.9.3" there because the driver "found" it there, or 
> > is it generated by the compiler version you have on your machine, and 
> > that's why you created the directory with "4.9.3" in it?
> > 
> > I'm worried that if it's the latter, the test will fail on any machine that 
> > doesn't have that compiler version. But I really don't know how it works, 
> > so it was an honest question. :)
> It's there because Driver founds it in Input directory structure. I don't 
> have that version installed locally, if that's what worries you. You could 
> say it's intentionally different from my install but I've simply copied the 
> version used by other Gentoo tests.
That answers my question, thanks! :)


https://reviews.llvm.org/D25686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25686: [Driver] Improve support for Gentoo arm*-hardfloat-*-*eabi triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.



Comment at: lib/Driver/ToolChains.cpp:1674
   TripleAliases.append(begin(ARMHFTriples), end(ARMHFTriples));
+} else if (TargetTriple.getEnvironment() == llvm::Triple::MuslEABIHF) {
+  TripleAliases.append(begin(ARMHFMuslTriples), end(ARMHFMuslTriples));

mgorny wrote:
> rengolin wrote:
> > You're not testing this...
> Well, I presumed I don't have to test every single possible triple. But sure, 
> I can add a test for that.
You should test every new thing that you add.



Comment at: test/Driver/linux-ld.c:1016
+// CHECK-LD-GENTOO-ARMHF: "-dynamic-linker" "/lib/ld-linux-armhf.so.3"
+// CHECK-LD-GENTOO-ARMHF: 
"{{.*}}/usr/lib/gcc/armv7a-hardfloat-linux-gnueabi/4.9.3/crtbegin.o"
+// CHECK-LD-GENTOO-ARMHF: 
"-L[[SYSROOT]]/usr/lib/gcc/armv7a-hardfloat-linux-gnueabi/4.9.3"

mgorny wrote:
> rengolin wrote:
> > where is 4.9.3 coming from?
> It's the version used in the input tree, i.e. the two added files above.
Right, I mean is this "4.9.3" there because the driver "found" it there, or is 
it generated by the compiler version you have on your machine, and that's why 
you created the directory with "4.9.3" in it?

I'm worried that if it's the latter, the test will fail on any machine that 
doesn't have that compiler version. But I really don't know how it works, so it 
was an honest question. :)


https://reviews.llvm.org/D25686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25686: [Driver] Improve support for Gentoo arm*-hardfloat-*-*eabi triples

[Renato Golin via Phabricator via cfe-commits]

rengolin added a reviewer: compnerd.
rengolin added inline comments.



Comment at: lib/Driver/ToolChains.cpp:1674
   TripleAliases.append(begin(ARMHFTriples), end(ARMHFTriples));
+} else if (TargetTriple.getEnvironment() == llvm::Triple::MuslEABIHF) {
+  TripleAliases.append(begin(ARMHFMuslTriples), end(ARMHFMuslTriples));

You're not testing this...



Comment at: test/Driver/linux-ld.c:1016
+// CHECK-LD-GENTOO-ARMHF: "-dynamic-linker" "/lib/ld-linux-armhf.so.3"
+// CHECK-LD-GENTOO-ARMHF: 
"{{.*}}/usr/lib/gcc/armv7a-hardfloat-linux-gnueabi/4.9.3/crtbegin.o"
+// CHECK-LD-GENTOO-ARMHF: 
"-L[[SYSROOT]]/usr/lib/gcc/armv7a-hardfloat-linux-gnueabi/4.9.3"

where is 4.9.3 coming from?


https://reviews.llvm.org/D25686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits