This is an archive of the discontinued LLVM Phabricator instance.

code refactoring on ARMTargetInfo class
ClosedPublic

Authored by labrinea on Jun 30 2015, 8:02 AM.

Details

Summary

Refactored ARMTargetInfo in to use existing API from llvm/lib/Support/TargetParser.cpp in order to extract target specific information.

Diff Detail

Event Timeline

labrinea updated this revision to Diff 28782.Jun 30 2015, 8:02 AM
labrinea retitled this revision from to code refactoring on ARMTargetInfo class.
labrinea updated this object.
labrinea edited the test plan for this revision. (Show Details)
labrinea set the repository for this revision to rL LLVM.
labrinea added a subscriber: Unknown Object (MLST).
rengolin edited edge metadata.Jun 30 2015, 1:27 PM

Hi Alexandros,

Thanks for the patch. I'm really happy that we can now change everything else and not need to change the TargetParser any more. This means we're stabilising the interface, and soon will be time to move it inside the new Tuple class.

About your patch, it's overall good, but I have a few minor comments inline.

cheers,
--renato

lib/Basic/Targets.cpp
4161

These could be cached, too, as members of the class.

4165

What if this goes wrong?

I guess all other methods in the class will start to return rubbish, but then again, that's what it currently does.

Maybe a future patch to cover the cases where the values are XX_INVALID on all the get methods and print some warning/error.

Also, you might want to extract this lines (and IsThumb / Atomic below) into a private method that both the constructor and setCPU call.

4394

You should also set IsThumb and ShouldUseInlineAtomic

4536

This variable name is odd...

Hi Alexandros,

Thanks for the patch. I'm really happy that we can now change everything else and not need to change the TargetParser any more. This means we're stabilising the interface, and soon will be time to move it inside the new Tuple class.

About your patch, it's overall good, but I have a few minor comments inline.

cheers,
--renato

Thanks for the feedback. I have already made some changes and I will upload a new patch soon. Regarding the odd variable name? (it was not quite clear what I should change)

lib/Basic/Targets.cpp
4536

Are you referring to "is32Bit"? Any suggestions?

rengolin added inline comments.Jul 1 2015, 3:23 AM
lib/Basic/Targets.cpp
4536

Yes. I'm not sure what this is trying to achieve, TBH, but getting "all 32-bit ARM architectures" is not. :)

Though, since this was there before, I guess it's ok to keep it for now. Feel free to add a FIXME comment.

labrinea added inline comments.Jul 1 2015, 8:05 AM
lib/Basic/Targets.cpp
4394

Setting IsThumb in setCPU causes regressions. It doesn't make sense to me but setCPU changes the ISA from thumb to arm.

rengolin added inline comments.Jul 1 2015, 8:09 AM
lib/Basic/Targets.cpp
4394

Right, this is a major bug and needs to be addressed. You shouldn't mix TargetParser changes with bug fixes, but would be good to add a FIXME comment and open a bug in bugzilla to describe the problem.

What about ShouldUseInlineAtomic?

labrinea added inline comments.Jul 1 2015, 8:47 AM
lib/Basic/Targets.cpp
4394

In the current implementation ShouldUseInlineAtomic is checking llvm::triple which is set in the constructor. Apparently, setCPU checks MaxAtomicInlineWidth (which is also set in the constructor depending on the value of ShouldUseInlineAtomic), thus takes into account the ISA from llvm::triple.

rengolin added inline comments.Jul 1 2015, 8:56 AM
lib/Basic/Targets.cpp
4394

It uses the ISA and the ArchVersion, which can change here.

labrinea updated this revision to Diff 28875.Jul 1 2015, 9:18 AM
labrinea edited edge metadata.
labrinea removed rL LLVM as the repository for this revision.

made some changes:

  • according to review suggestions
  • getCPUAttr() was causing regressions when returning nullptr
  • the API from TargetParser was causing regressions when llvm::triple subArch was not specified
rengolin added inline comments.Jul 2 2015, 2:09 AM
lib/Basic/Targets.cpp
4160

This seems quite arbitrary... Couldn't you return an empty string?

labrinea added inline comments.Jul 2 2015, 2:32 AM
lib/Basic/Targets.cpp
4160

If no subArch is specified then setArchInfo() cannot do much with an empty string. ArcKind will be set to invalid and getCPUAttr() will return an empty string. A regression will appear since ARM_ARCH_6J won't be defined as expected. That ("armv6j") should be the corresponding subArch for the default CPU ("arm1136j-s") specified in the constructor.

rengolin added inline comments.Jul 2 2015, 2:40 AM
lib/Basic/Targets.cpp
4160

Then, use the default value in the constructor (CPU) instead, to figure out the arch.

Don't just hard-code yet another temporarily matching value.

labrinea added inline comments.Jul 2 2015, 2:44 AM
lib/Basic/Targets.cpp
4160

If it is more maintainable instead of setArchInfo("armv6j") we could call setCPU(CPU) (where cpu is "arm1136j-s" by default) in case subArch is not defined.

rengolin added inline comments.Jul 2 2015, 3:00 AM
lib/Basic/Targets.cpp
4160

or you could use TargetParser::parseCPUArch(CPU);

Anything bur more hardcoded values.

Ok, fair enough. Thanks again for the feedback :) I am currently trying to replace some of the if-else statements in getDefaultFeatures() that extract FPU information based on the CPU. I am adding a helper function in TargetParser for that. It maps CPU names to FPUKinds. Shall I commit the current patch first, or altogether?

Ok, fair enough. Thanks again for the feedback :) I am currently trying to replace some of the if-else statements in getDefaultFeatures() that extract FPU information based on the CPU. I am adding a helper function in TargetParser for that. It maps CPU names to FPUKinds. Shall I commit the current patch first, or altogether?

I think you should commit this small fix now, and then refactor it later if needed, with your next patch.

Thanks!

labrinea updated this revision to Diff 28940.Jul 2 2015, 4:52 AM
labrinea set the repository for this revision to rL LLVM.

Updated, shall I commit it?

Yup, looks good to me, now. Thanks!

--renato

rengolin accepted this revision.Jul 2 2015, 5:51 AM
rengolin edited edge metadata.
This revision is now accepted and ready to land.Jul 2 2015, 5:51 AM
labrinea updated this revision to Diff 28964.Jul 2 2015, 11:07 AM
labrinea edited edge metadata.

The previous patch was causing regressions because ShouldUseInlineAtomic was set incorrectly when subArch was not specified. My mistake that I did not notice it before the commit. Again, the fact that setCPU() can change the Arch that was previously defined by llvm::triple doesn't make sense to me. That is the reason that ShouldUseInlineAtomic and IsThumb are initialized in the constructor and never change again. Anyway, I added some FIXME labels. Shall I commit?

If this makes the tests pass, I'm ok with it, for now. But that part is getting uglier. We should understand what's causing the regressions and fix it as soon as possible.

Thanks, please commit.

rengolin closed this revision.Jul 3 2015, 4:59 AM
labrinea updated this revision to Diff 29533.Jul 13 2015, 1:41 AM
labrinea removed rL LLVM as the repository for this revision.

Hi Alexandros,

Thanks again for going through this. I have a number of comments, mainly regards organization. This class is a bit messy, and since we're touching most of it's caching logic, I think it's fair if we do a bit of cleaning up.

cheers,
--renato

lib/Basic/Targets.cpp
33

Why do you need to include <locale>?

4146

Since we're setting up the default CPU on the Arch, we don't need it to be initialised here.

Also, "arm1136" looks like the wrong value here, since the default CPU for ARM when nothing is known is "arm7tdmi".

I risk to say that this value was never taken into consideration because the architecture was never empty (errors dealt with before), so I'd leave CPU uninitialized here, and assert down there if CPU == nullptr.

4165

Maybe we should have two setArchInfo():

setArchInfo(ArchName);

Called here, and set when the Arch changes. And:

setArchInfo(CPUName);

set from setCPU() below. This version doesn't change the Arch, only the CPU affected parts of the cached parameters.

The Arch version should call the CPU version to remove redundancies in the code.

4168–4170

This should also go inside setArchInfo()

4170

ShouldUseInlineAtomic must be set up inside setAtomic(), too.

4350

I'm not why you're changing the name of this function, but it seems it's rather a private matter (no use outside of this file).

Since we're caching all properties of the target, I suggest you move this to private and call it on setArchInfo() only and keep a local copy of it.

4358

This validation doesn't belong here. If ARMTargetParser returned a valid CPU name, the name is valid.

If the name is wrong, ARMTargetParser is wrong and should be fixed instead.

4390

Ah, yes, this makes a lot of sense!

4401

if you cache CPUAttr, this call loses its argument.

4407

if you cache CPUAttr, this call loses its argument.

4408

if you cache CPUAttr, this call disappears.

labrinea added inline comments.Jul 13 2015, 3:40 AM
lib/Basic/Targets.cpp
33

I thought isalnum needed that.

4146

The value "arm1136j-s" was taken into consideration. A regression will appear if we don't set it, since ARM_ARCH_6J won't be defined as expected (tools/clang/test/Preprocessor/init.c). I don't like this hard-coded string either but it was there before my patch. Do you think we should change the test?

4168–4170

IsThumb depends on ArchISA. ArchISA should be initialized based on triple and never change. Updating IsThumb in setArchInfo() will not change its value.

4358

That's the assertion I introduced to catch the buildbot error (whitespace expected after macro definition). I am still surprised that it passed the test this time since the only thing I added is three cases to the switch clause (ARMV6HL, AK_ARMV7L, AK_ARMV7HL). Those were the only cases of illegal characters ( '-' ) in CPUAttr not handled.

rengolin added inline comments.Jul 13 2015, 6:05 AM
lib/Basic/Targets.cpp
4146

No. Add another FIXME outlining the problem, hinting where to fix it properly. This patch is already too big and long lasting for its own good. :)

4168–4170

Good point.

4358

Regardless, this is not the place to check for this kind of problem.

Maybe, on a separate patch, we can add that check to ARMTargetParser, which is the right place.

Also, using std::string is a bit heavy handed for this task.

labrinea added inline comments.Jul 13 2015, 7:12 AM
lib/Basic/Targets.cpp
4170

Moving it there triggers regression (tools/clang/test/CodeGen/atomics-inlining.c). When triple does not define a subArch, (and therefore llvm::ARMTargetParser::getDefaultCPU(ArchName) returns null), the value of ShouldUseInlineAtomic should be false according to the logic before my patch. That's because ShouldUseInlineAtomic was parsing the arch name and returned true only if archName started with "armv" or "thumbv".

4176

If we are finally keeping the hard-coded string for CPU with a FIXME label, there is no point in having setArchInfo(ArchName). It is redundant. We still have to call setArchInfo(CPU) to avoid the regressions.

rengolin added inline comments.Jul 13 2015, 7:17 AM
lib/Basic/Targets.cpp
4170

But by the time you run setAtomic() you already have Arch, CPU and everything. And if you don't, then check for nullptr and set atomic to false.

4176

True. And since this is the constructor, and the Arch is not supposed to change, it should be fine to keep the Arch part of it, here.

labrinea added inline comments.Jul 13 2015, 7:38 AM
lib/Basic/Targets.cpp
4170

The tricky part is that these variables (Arch, CPU, etc..) are set both in the if and the else path, regardless if the triple specifies a sub arch or not. What I could do is to cache DefaultCPU, which is set by llvm::ARMTargetParser::getDefaultCPU(ArchName), and test that for nullptr value in setAtomic().

rengolin added inline comments.Jul 13 2015, 7:44 AM
lib/Basic/Targets.cpp
4170

Yes, that could work.

labrinea updated this revision to Diff 29577.Jul 13 2015, 8:35 AM

I have updated the revision in case phabricator did not send any notification.

Is it approved for commit?

Not yet, let me check over all concerns again.

Hi Alexandros,

This now looks good to go. Thanks for the patience! :)

Have you tested on ARMv7, self-hosting? IIRC, that's where it was failing last time.

cheers,
--renato